Skip to content

fix(agc): retry failed host imports after range replacement - #1357

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
TibixDev:pr/shared-import-spans
Oct 12, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
TibixDev:pr/shared-import-spans

Conversation

@TibixDev

@TibixDev TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What

Retry a rejected host import when any registered owner of its span is replaced at the same address, or when the span changes. Failed decisions retain weak owner identities and their full extent, so they cannot pin freed allocations. An unchanged failure stays cached across unrelated mapping changes.

Narrowed to the remaining correctness issue on main d146ec1b6: this is now two files, independent of #1324. Main already contains import retirement/lifetime handling, adjacent-range imports (#3158), Windows alias fallback (#3324), and aligned import windows (#3425); those implementations are preserved. The earlier stacked span-import optimization and its performance claims are removed from this PR. No duplicate ClearHostImports or stale Windows early-return implementation remains.

Tested

Fedora 44, GCC 16 Release. The new graphics fixture injects a failed Vulkan allocation, checks repeated failures and an unrelated mutation do not retry, replaces either the first or second span owner at the same address, and checks the retry succeeds without pinning the old owner. It also covers shrinking a failed span to its surviving range.

With unchanged main GuestBufferMemory.cpp and this fixture, graphics tests fail with same-address remap retained a failed import decision. Restoring the fix passes. Six focused CTests pass: graphics, guest memory, import unmap, storage upload lifetime/failure, and descriptor pool recycling. The Vulkan integration cases ran on RX 6600/RADV with core/synchronization validation; the injected-failure fixture uses mock Vulkan. Conventions and whitespace checks pass.

No title speedup or Windows result is claimed. A separate current-main Skyrim boot stops at unsupported NGS2 reverb channel conversion before a usable frame-time comparison; it does not establish title impact of this fix. Full platform CI and maintainer re-review remain.

Checklist

  • Main d146ec1b6; overlapping upstream work checked.
  • One topic: failed-import invalidation after remapping.
  • No new code comments.
  • Unsupported paths still throw; UI stubs are documented.
  • No notes, logs, or images in the repository.
  • No new third-party code.
  • General behavior.
  • Depends on: none.
  • AI-assisted: yes.

@TibixDev

TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased on eaa6ccea. Integrated #1051’s Linux writable-range check with the owner-aware failure cache. This fixes the compile error in GitHub’s merge build. Kept #1113’s byte budget and the import lifetime/remap fixes.

Fedora 44 Release: agc_driver_graphics_tests, guest_memory_tests, agc_driver_recorder_tests, agc_recorder_kept_bytes_tests build; the targeted tests and contribution checks pass. Windows CI is pending.
RADV and lavapipe: --shared-import-only pass with core and synchronization validation; no validation errors.

@TibixDev
TibixDev force-pushed the pr/shared-import-spans branch 2 times, most recently from 7076406 to 23e69a9 Compare October 8, 2026 07:54
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 8, 2026
@TibixDev
TibixDev force-pushed the pr/shared-import-spans branch from 23e69a9 to 04bc4e5 Compare October 8, 2026 08:04
@popcoder565

Copy link
Copy Markdown
Contributor

Heads-up: this branch no longer compiles when merged onto main (I tried 30e89cc while building #1359 on Windows to test it). #1239 merged at 09:27 UTC, after this branch's last CI run, and added its own ClearHostImports(VkDevice) to GuestBufferMemory.cpp (L1121 on main). The merge is textually clean, so GitHub still shows it as mergeable, but the build fails:

  • redefinition of 'void AgcDriver::Graphics::ClearHostImports(VkDevice)', because both versions are present.
  • main's version calls state.destroyBuffer and state.freeMemory, which this branch's HostImports no longer has.
  • On Windows, main's version also unmaps entry.alias.

#1359 carries the same commits, so it fails the same way. Since the description says this supersedes #1239, I think the rebase only needs main's copy removed or folded into this branch's release path. I haven't tested anything further on Windows.

@Angel21PC

Copy link
Copy Markdown
Contributor

On Windows, spans are still refused here (importRange returns nullptr under _WIN32). Part of what they need there doesn't depend on this PR, so I opened #2254: a read-write alias of shared views that are not one contiguous run of a section is mapped run by run into a placeholder reservation instead of being refused.

Once both are in, the _WIN32 early return can go. A span whose alias still fails (mixed private and shared pages) would then need to be marked failed instead of throwing. I can send that follow-up, unless you'd rather fold it in here.

In Poppy Playtime: Chapter 4 (UE5) on Windows, views crossing adjacent registered ranges were being copied (about 158 GB per 10 s of gameplay). Importing them over the span cut that to about 260 MB of reads and roughly doubled the frame rate (local build with other open PRs).

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work here. This can't be merged as is:

  • It is stacked on #1324, which is still open and now conflicts with main itself (GuestBufferMemory.cpp). Nothing here can land before it.
  • On top of that, this head conflicts with main in VulkanDevice.cpp, GuestBufferMemory.{hpp,cpp}, Texture.cpp, tests/Recorder.cpp, GuestAllocations.{hpp,cpp}, DirectMemory.cpp, tests/GuestMemory.cpp and TechnicalDebt.md. These touch the import/lifetime logic, so they need the author to resolve them, not a mechanical rebase.
  • Windows: spans are still refused (importRange returns nullptr under _WIN32), see @Angel21PC's comment and #2254. Please say how this PR and #2254 combine, or keep the limitation documented in TechnicalDebt.
  • @oneandonlydean measured #1324 alone with no frame-time change; the same question applies to this stack: please name the title and scene where spanning imports gives a measurable gain (before/after numbers).

Please rebase once #1324 is merged (or fold the stack in a way that keeps one topic per PR) and we will review it again.

Retain weak identities and the full failed span so a replacement of either owner or a shorter remaining run retries the import. Keep unchanged failures cached across unrelated mapping changes without pinning their allocations. Existing upstream import lifetime, span windows and Windows fallback remain in place. AI-assisted implementation.
@TibixDev
TibixDev force-pushed the pr/shared-import-spans branch from 04bc4e5 to 2441a4b Compare October 12, 2026 04:37
@TibixDev TibixDev changed the title fix(agc): preserve shared buffer imports across remaps fix(agc): retry failed host imports after range replacement Oct 12, 2026
@TibixDev

Copy link
Copy Markdown
Contributor Author

@adriadam10 @popcoder565 Narrowed and rebased onto main d146ec1b6: this is now only the failed-import remap fix (two files), with no #1324 prerequisite. Main already supplies retirement/lifetime handling, span imports (#3158), Windows alias fallback (#3324), and aligned windows (#3425). Those stay intact; the old stack, duplicate teardown and span-performance claims are removed.

The remaining bug reproduces independently: a failed import is keyed only by base address, so replacing either range in a span can leave the new backing permanently rejected. The new fixture injects a failure, checks unchanged/unrelated mappings keep it cached, then replaces each owner and shrinks a span. Unchanged main fails same-address remap retained a failed import decision; the fix passes and weak failure records do not pin old owners.

Six focused CTests plus conventions/whitespace checks pass on Fedora 44; native integration cases ran on RADV with core/synchronization validation. No fresh Windows or title-speedup claim. Full platform CI and re-review remain. AI-assisted implementation and validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants