Skip to content

perf(agc): reuse import decisions across mapping changes - #1361

Closed
TibixDev wants to merge 10 commits into
boykopovar:mainfrom
TibixDev:pr/import-mapping-cache
Closed

TibixDev wants to merge 10 commits into
boykopovar:mainfrom
TibixDev:pr/import-mapping-cache

Conversation

@TibixDev

@TibixDev TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Status after review

Awaiting title benefit and a consumer-led redesign.

The description now calls out the full legacy integration stack raised in review: #1324, #1357, #1359 and #1360. This does not make the old embedded copies equivalent to today's branch heads. In this pass, #1357 and #1359 were rebuilt independently on main; #1357 is now specifically a failed-import remap-identity fix. The old stack must not overwrite main's later span/import-lifetime work.

The 111–164 ns → 16–18 ns repeated-failure lookup result remains a microbenchmark, with no isolated frame-time result. The new Skyrim loading run does show failed host imports, but it does not establish that repeated decision lookup is a bottleneck: its measured resource cost is dominated by upload. If a title profile shows that cost, the next version should include #1360's first real consumer and compare against main's eager map observer. No code push or claimed gain from merely observing failures.

What

Reuse failed import decisions for unchanged allocation ranges and exact spans across mappings. Use range history before acquiring the full registry. Retain existing owners when history expires; discard decisions when a backing changes. Successful imports still take priority.

Tested

Earlier branch-specific tests and measurements remain in the discussion. They have not been rerun on a redesigned current-main version of this PR. The review audit and shared NGS-assisted title exploration do not establish that this head is merge-ready. No full Linux/Windows platform CI was dispatched in this pass.

Checklist

@github-actions github-actions Bot added libc libkernel build docs agc libSceAgc* needs rebase Conflicts with main, rebase needed labels Oct 8, 2026
@TibixDev
TibixDev force-pushed the pr/import-mapping-cache branch from ae1b0b5 to 6a61c81 Compare October 8, 2026 07:28
@TibixDev

TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased on eaa6ccea. Updated #1357 with #1051’s Linux writable-range check and owner-aware failure caching. This fixes the merge-build compile failure while retaining the byte budget and existing ordering/remap behavior.

Fedora 44 Release: agc_driver_graphics_tests, guest_memory_tests, guest_mapping_history_tests, guest_time_conversion_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, --failed-imports-only pass with core and synchronization validation; no validation errors.

@TibixDev
TibixDev force-pushed the pr/import-mapping-cache branch from 6a61c81 to a0165b0 Compare October 8, 2026 07:56
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 8, 2026
@popcoder565

Copy link
Copy Markdown
Contributor

Tested on Windows, since the description says it wasn't: Windows 10 (19045), RTX 3060 Laptop (driver 616.92), MinGW-w64 GCC 15.2.0, Release, main eaa6cce merged with a0165b0.

Use a locked ordered lookup for one containing readable allocation when the import registry is current. Recheck the registry generation after acquiring the lease, and retain the full reconciliation path when mappings change.

Tests cover bounds, gaps, read-only and inaccessible ranges, address reuse, changed protection, pinning the requested allocation, and releasing an unrelated allocation. agc_driver_graphics passes on Fedora 44. The preceding Skyrim profile attributed 76% of sampled CPU cycles to full allocation leases.

Written with AI assistance.
Bound allocation coverage and availability lookups, stage only changed mapping fragments before host operations, and retain atomic registry updates on host failure. Descriptor uploads acquire a targeted lease when an unchanged registry has no cached import.

Validation: agc_driver_graphics and guest_memory passed, including host-failure rollback, gaps, split mappings and unrelated leases. A 20000-entry mapping microbenchmark dropped from 2354 ms to 0.764 ms for 1000 localized operations; this is not a game benchmark. AI-assisted implementation.
Track weak allocation identity for successful and failed imports so same-address backing replacements retire old memory and retry failures. Add shared-memory roundtrip and remapping coverage and identify the import path in diagnostics.

Validated guest-memory and graphics tests plus software Vulkan tests. RX6600 native dma-buf roundtrip passed with Khronos synchronization validation. Written with AI assistance.
Combine adjacent backing slices through UDMABUF_CREATE_LIST, retain every allocation identity for remap invalidation, and preserve overlapping smaller imports. Publish overlapping unit shadows before disabling them for combined bindings.

Validated guest-memory and graphics suites, CPU Vulkan remap/shadow regressions, and RX 6600 dma-buf cross-backing reads and writes with Vulkan synchronization validation. Game imports remain opt-in pending the unresolved loading fault.

AI-assisted implementation and review.
Keep import records and their Vulkan allocations together through prepared
descriptors, staged copies, and indirect commands. Reject retired bindings
when deriving resource-cache proofs, account for pinned imports until final
release, and clear retained imports and draw writers before device teardown.

Add a fixed-remap regression for descriptor stability, stale cache identity,
pending batch ownership, and eventual release. Include GPU addresses in the
existing opt-in import trace for fault diagnosis.

Validation: Fedora 44 GCC 16 Release build; llvmpipe shared-import and
copied-direct tests with Vulkan synchronization validation, both normally
and with APS5_SHADOW_VERIFY=1; RX 6600 shared-import tests with synchronization
validation, including RADV_DEBUG=hang; agc_driver_graphics and guest_memory
CTest targets passed. The unrelated full recorder suite was not passed.

The Skyrim GPU reset remains unconfirmed as a consequence of this bug.
AI-assisted implementation and review.

@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, #1357, #1359 and #1360. All four are still open and conflict with main, so nothing here can land before them. The Depends on line lists only #1357 and #1360; please list all of them.
  • This head also conflicts with main (GuestBufferMemory.cpp, libc/CMakeLists.txt, GuestAllocations.{hpp,cpp}), in the replaceRange / host-import lookup logic, so it needs the author rather than a mechanical rebase.
  • The gain is a lookup microbenchmark (111–164 ns → 16–18 ns per repeated failed span). Please name a title and scene where repeated failed imports show up in a frame, with before/after frame times; on #1324 @oneandonlydean measured no frame-time change and that question is still open.
  • The caching relies on GuestAllocationsValidateMapping_nid_no_patch from #1360, which has no caller there; it should land together with this consumer or the stack should be folded so each PR keeps one topic.

Please rebase once the base PRs are merged and we will review it again.

@TibixDev

Copy link
Copy Markdown
Contributor Author

Review-pass update:

The description now calls out the full legacy integration stack raised in review: #1324, #1357, #1359 and #1360. This does not make the old embedded copies equivalent to today's branch heads. In this pass, #1357 and #1359 were rebuilt independently on main; #1357 is now specifically a failed-import remap-identity fix. The old stack must not overwrite main's later span/import-lifetime work.

The 111–164 ns → 16–18 ns repeated-failure lookup result remains a microbenchmark, with no isolated frame-time result. The new Skyrim loading run does show failed host imports, but it does not establish that repeated decision lookup is a bottleneck: its measured resource cost is dominated by upload. If a title profile shows that cost, the next version should include #1360's first real consumer and compare against main's eager map observer. No code push or claimed gain from merely observing failures.

I have updated the PR description to match this status. The PR remains open; the outstanding review requirements are not being marked resolved. AI-assisted audit.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Closing for now: only a microbenchmark is shown, no title. With the queue this large we're closing stalled perf PRs that don't show a gain in a title; reopen it with before/after numbers from a title scene (the summary lines from the performance report form) if you pick it up again.

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

Labels

agc libSceAgc* build docs libc libkernel needs rebase Conflicts with main, rebase needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants