Skip to content

perf(graphics): retain resource binding plans - #1315

Closed
TibixDev wants to merge 1 commit into
boykopovar:mainfrom
TibixDev:pr/resource-binding-plans
Closed

TibixDev wants to merge 1 commit into
boykopovar:mainfrom
TibixDev:pr/resource-binding-plans

Conversation

@TibixDev

@TibixDev TibixDev commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Status after review

Awaiting a measured title benefit.

Current buildPrepare/buildComplete uses compact bindings and runtime image metadata. The old retained plan must be redesigned around that split; its old structures cannot simply be rebased into the new path.

The existing measurements remain negative or inconclusive: our A/B ranges overlap, and the review profile was 19 ms on main versus 21–22 ms with the PR per 6,000 builds. I do not have a title/scene that this change demonstrably speeds up. In the new main + experimental NGS Skyrim loading sample, bindings took 2 ms across 257 resource builds while guest upload took about 10.6 seconds. That sample is not a controlled frame-time comparison and does not justify this cache. The body now treats earlier fixture numbers as historical, not evidence of a current gain. No speculative 500-line port or merge-ready claim.

What

Retain descriptor layouts, allocation counts, and buffer patch positions per shader variant. Each draw still resolves live resource addresses and owns its resource lifetime.

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

  • Current-main implementation and outstanding review requirements complete.
  • Isolated title benefit / reference validation established where required.
  • Depends on: none; current preparation-path redesign pending.
  • AI-assisted: yes.

Retain descriptor layouts, allocation counts and buffer patch positions
per shader variant. Resolve live addresses on each resource build and
preserve independent resource lifetimes. Bound plan retention and bypass
unknown variant identities.

Cover changing addresses, array bindings, push offsets, eviction and plan
lifetime. Supply the sampled-image metadata required by the resource-build
benchmark. Fedora 44 Release driver, graphics and PM4 tests pass. RX 6600
validation passes; llvmpipe reports an upstream write-after-write warning.
The isolated resource-build benchmark does not establish a clear gain.

AI-assisted: yes.
@TibixDev
TibixDev force-pushed the pr/resource-binding-plans branch from 47e2a47 to 71d4800 Compare October 8, 2026 05:34
@TibixDev

TibixDev commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on 5850a6cd. I compared that upstream with upstream plus this PR alone, using the same resource-build fixture. I fixed the fixture to supply the image-sampler metadata required by current upstream.

Fedora 44 Release, GCC 16, Ryzen 5 5600 pinned to CPU 3, RX 6600 with host RADV. Eight runs per variant in ABBA order. Each run had one warmup and nine timed batches of 1,024 resource builds. The fixture uses eight buffers, rotates the first buffer address, and disables host and DMA-BUF imports. Validation was disabled during timing.

Times in microseconds per resource build; medians of the eight run medians:

Textures Upstream This PR Reduction Upstream run range PR run range
0 515.5 510.1 1.1% 511.5–537.4 504.8–516.2
8 574.7 572.5 0.4% 559.9–606.2 554.5–587.1

Checksums match. The ranges overlap, so this fixture does not establish a clear isolated performance gain. The earlier combined-fork figures are not a substitute for that evidence. Title frame time was not measured.

Driver, graphics and PM4 tests pass. Resource, snapshot and live-input checks pass on RX 6600 with core and synchronization validation. On llvmpipe, both upstream and this PR report the same SYNC-HAZARD-WRITE-AFTER-WRITE at vkCmdUpdateBuffer for offset 0, size 16. I have not counted that as a clean validation pass. Windows was not tested.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

#639 rewrote buildPrepare and buildComplete (compact bindings, runtime image metadata), so this needs a rebase. Before that: your fixture shows no gain, and a profiled run here didn't either (bindings phase 19 ms on main vs 21-22 ms with this, per 6,000 builds). Which title does it speed up?

MichalSy added a commit to MichalSy/AnyPS5 that referenced this pull request Oct 10, 2026
Adapt upstream PRs boykopovar#1315, boykopovar#2791 and boykopovar#2792 to the current descriptor ABI and synchronization paths. Bound immutable binding plans to 1024 entries while retaining live resource addresses and specialization identity.

Keep real CPU writebacks and fault completion active. Capture generation baselines only over settled guest bytes. Add focused cache, texture baseline and copy synchronization regressions.

@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 rebase and the A/B fixture. Two blockers:

  • Conflicts with main: #639 rewrote buildPrepare/buildComplete (compact bindings, runtime image metadata). The PR now conflicts in ShaderResources.cpp, ShaderResources.hpp, libSceAgcDriver/CMakeLists.txt, tests/Graphics.cpp and tests/Recorder.cpp. Please rebase and rerun the PR's tests on top of the new preparation path.
  • No measured gain: your own numbers overlap (515.5 → 510.1 µs and 574.7 → 572.5 µs, ranges overlapping), and @oneandonlydean's profile shows the bindings phase at 19 ms on main vs 21-22 ms with this PR per 6,000 builds. A perf PR needs before/after numbers that show a gain. Please answer his question: which title does it speed up, and by how much (frame time or the bindings phase in that title)?

If the rebased version still shows no gain, please close it; a 500-line change to resource binding without a measured benefit is not worth the maintenance. #1318 is stacked on this one and will need the same rebase.

@TibixDev

Copy link
Copy Markdown
Contributor Author

Review-pass update:

Current buildPrepare/buildComplete uses compact bindings and runtime image metadata. The old retained plan must be redesigned around that split; its old structures cannot simply be rebased into the new path.

The existing measurements remain negative or inconclusive: our A/B ranges overlap, and the review profile was 19 ms on main versus 21–22 ms with the PR per 6,000 builds. I do not have a title/scene that this change demonstrably speeds up. In the new main + experimental NGS Skyrim loading sample, bindings took 2 ms across 257 resource builds while guest upload took about 10.6 seconds. That sample is not a controlled frame-time comparison and does not justify this cache. The body now treats earlier fixture numbers as historical, not evidence of a current gain. No speculative 500-line port or merge-ready claim.

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: no title or measured gain is shown, as the author noted. 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 needs rebase Conflicts with main, rebase needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants