Repository navigation
Conversation
…f its key The draw key covers the user data the stages read, which in the menu of Uncharted: Legacy of Thieves Collection (PPSA05684) holds per-draw buffer addresses: of about 45,000 lookups per 10 s none hit, yet every draw built its stage variants (the captured words copied and sorted) and inserted them, evicting a 4096-entry cache over and over. A key that has no entry is now only remembered (16384 slots of key hashes) the first time it is seen, and its draw is inserted when it comes again; titles whose keys repeat are cached one draw later. The [draw-cache] line counts the first sightings left out. APS5_DRAW_CACHE_ADMIT_ALL=1 inserts every draw as before. The draw-cache reuse test now issues its draw three times: the third one hits. Menu: 7,688 first sightings left out and 209 inserts per 10 s; the key, lookup and validate phase went from 5.4 to 2.4 us per draw.
|
Two keys that land in the same sighting slot overwrite each other ( The early return at |
Dextroid17
left a comment
Dextroid17
left a comment
There was a problem hiding this comment.
Adding to @oneandonlydean's comment above rather than repeating it. Both of their points are correct, and I reproduced the second from outside: with only DrawCapture.cpp's call site and DrawCache.cpp's admitDrawEntry reverted (headers kept so the test still compiles), agc_driver_draw_cache_reuse_tests exits 1 on the test's own message the three draws made 3 draw-cache lookups (1 without an entry), expected 3 lookups with 2 absent, while the PR head passes on the CI device (lavapipe). What I can add is the cause, the fix, and three things neither comment covers.
1. Cause of the colliding keys: the index reads 28 of the 64 key bits. (key ^ (key >> 29)) % 16384 mixes bits 0–13 with bits 29–42; bits 14–28 and 43–63 can never change the slot. Flipping each bit of a key one at a time: 28 bits can move the slot, 36 cannot, and the collision is therefore systematic for a whole key class rather than birthday chance:
0x0000000000001111 -> slot 4369
0x000000001fffd111 -> slot 4369 (differs only in bits 14..28)
0x0010000000001111 -> slot 4369 (differs only in bits 43..63)
A whole-word mix before the mask removes that class: k *= 0x9E3779B97F4A7C15ull; slot = (k >> 50) % SightingSlots; — measured, it takes the count of slot-influencing bits from 28 to 64. It is one multiply, and it does not remove birthday collisions (thousands of live keys in 16384 slots still collide), so it fixes the systematic class, not all of them.
2. On "never inserted" — a refinement, not a disagreement. For the strictly alternating pair in the example, neither is ever admitted, exactly as stated: each sighting re-arms the slot to the other key. With a less regular interleaving the re-arm makes it a delay rather than a loss — at the rate the description measures (7,688 first sightings per 10 s ≈ 769/s, assuming the writes spread across the slots) the expected slot lifetime is 16384/769 ≈ 21 s, so a key whose sightings are much further apart than that is usually re-armed before it is seen again. That matters for sizing the regression: a hard "never cached" is the degenerate case, not the general one.
3. The one-line fix for the phase. DrawPhaseTiming::Phase(X) charges the elapsed lap to X and restarts it (core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawTiming.cpp:35-40); cacheDrawStages opens the lap at DrawCapture.cpp:87 and closes it at :130, and on the skipped path the next Phase() call is in Draw.cpp (:236/:268), so the slice lands under "vectors". Adding phaseTiming.Phase(DrawRowKeyLookupValidate); immediately before the early return makes before/after per-phase numbers comparable, and the figure in the description is that phase by name (DrawTiming.hpp:13), so it is the line to add before quoting it publicly a second time.
4. A test nit neither comment has. The new firstSightings == 1 expectation assumes the APS5_DRAW_CACHE_ADMIT_ALL override is unset; with it exported the same test fails on its own absent == 2 assertion (1 without an entry), which reads as a regression rather than as the override working. Either skip those two expectations when the variable is set, or pin it as a test property the way agc_shader_frontend_pair_off pins its flag.
5. Merge order, and explicitly no claim of a clash. Open #494 edits five of these same six files (DrawCache.hpp, Driver.hpp, DrawCache.cpp, DrawCacheStatistics.cpp, DrawCapture.cpp — not the test). The overlap bot has flagged nothing on this PR, so treat that as a rebase-when-it-lands note, not as a conflict.
Not verified, so it is not claimed. The perf figures need the title and a tree carrying #757/#758, so nothing here speaks to 5.4 → 2.4 µs beyond the accounting point above; and rendering equivalence is not claimed — no title on this machine. What I did check on that side: the only consumer of fresh past the call is Draw.cpp:362, and :363 clears an all-nullptr vector, so a first-sighting draw takes the full draw path with no recipe, which is also what it did before (an entry created in that same call could never have had a recipe).
Hermes Agent 🤖 at your service
|
Thanks @oneandonlydean and @Dextroid17, both points were right. I fixed them locally: sightings now go in 4-way buckets indexed by a multiplicative mix of the whole key, the early return is charged to Uncharted: Legacy of Thieves Collection (PPSA05684) menu, same build, alternating unprofiled runs, fps / median frame over the last 120 s:
There's no gain beyond noise: the per-phase saving doesn't show up at the frame level. So I'm closing this rather than asking for more review time on it. Thanks again for the careful review. |
3 participants
What
The draw key covers the user data the stages read. In the menu of Uncharted: Legacy of Thieves Collection (PPSA05684) that user data holds per-draw buffer addresses, so a key almost never repeats: of about 45,000 draw-cache lookups per 10 s, none hit. Yet every draw built its stage variants (the captured words, copied and sorted) and inserted them, evicting the 4096-entry cache over and over.
A key with no entry is now only remembered the first time it is seen, in 16384 slots of key hashes, and its draw is inserted when the key comes again. Titles whose keys repeat are cached one draw later. The
[draw-cache]line counts the first sightings left out.APS5_DRAW_CACHE_ADMIT_ALL=1inserts every draw as before.The draw-cache reuse test now issues its draw three times and expects the third to hit, plus one first sighting left out.
#494 attacks the same waste from the other side: draws that differ only in user data share their entry. This PR does not depend on it.
Tested
Linux, RTX 4070 Ti SUPER. Full ctest on this branch: all tests pass,
check_conventions.pypasses.Uncharted does not run on plain
main(it needs the image tables of #757/#758). On a local branch that carries them, in the menu: 7,688 first sightings left out and 209 inserts per 10 s, and the key/lookup/validate phase went from 5.4 to 2.4 µs per draw.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)