Repository navigation
perf(agc): query unit shadows without copying the overlap list - #856
Conversation
|
Thanks. One thing before it can go in. Performance PRs need before/after numbers for the change itself (the owner asked for this on #218). The 48.0 to 43.6 ms figure is for 15 changes together. Could you measure |
|
Measured it alone and put the numbers in the description: the overlap list building and its allocation (16 of 5,119 AGC worker samples, about 0.3%) are gone, and what is left under |
Dextroid17
left a comment
There was a problem hiding this comment.
Built and ran it — external PRs get no CI here.
Linux (CachyOS, GCC, Release), NVIDIA RTX 3080 Ti:
cmake --build build --parallel 16
ctest --test-dir build -j 16 → 359/359 passed
The rewrite is behaviour-preserving. AnyShadowedOverlaps returns true on the first live unit, which is exactly what the old loop did — the old body pushed into a vector and then scanned it, with no side effect after the push, so the early exit cannot change which unit is found. The guard (end <= address || byBase.empty()) and the upper_bound/--it walk are unchanged, so the search space is identical.
Two details that make the equivalence complete rather than approximate:
- The
overlapping()wrapper passes a visitor that always returnsfalse, so it still walks the whole range inbyBaseorder and builds the list in the same order for callers that need it. Nothing that consumes the vector sees a different ordering. - The span overload (
AnyShadowedOverlaps(std::span<...>)) delegates to the address overload in a loop and short-circuits on the first true, so it inherits the change without modification — I checked it rather than assumed it, since that was the one place the treatment could have been left half-applied.
The win is the shared_ptr copies. Naming it explicitly because it is the more interesting half: the old path copied a shared_ptr per overlapping shadow, and each copy is an atomic increment plus, on destruction, a decrement. Removing the vector removes those atomics per query, not just a small allocation. That is a real cost in a path that runs per guest store.
What I could not do: measure it. There is no benchmark here that isolates AnyShadowedOverlaps, so what I have is equivalence and no regression, not a number — I would rather say that than quote a timing I did not take.
This is a comment, not an approval — I have no merge rights here.
Written with AI assistance.
|
Checked the diff (same guard and |
What
AnyShadowedOverlapsis asked on every guest access the flush hook sees and on several draw and copy paths. It collected the overlapping shadows into astd::vector<std::shared_ptr<UnitShadow>>(an allocation, and a reference count increment and decrement per shadow) only to read them under the registry lock it already holds. It now visits them in place throughanyOverlapping, whichoverlapping()also uses for the callers that need the list.Tested
Windows 11, MinGW GCC 15.2, RTX 2080 Ti. This branch builds, and the 165 agc driver, recompiler and execution test targets pass with it stacked on the other changes I'm sending.
Measured alone on Sonic Origins Plus's heaviest menu page (about 1,650 draws per frame), as asked. The base is
mainat this PR's base (5895ea7) plus the commits my branch needs to boot this title and to sample it (a launcher script, the title's boot fixes, the process sampler; none of them are in this PR), against the same base with this commit on top. Three interleaved rounds, each the median of six FPS readings:Ahead in every round, though the gap is larger than the profile supports, so the profile is the better figure: over two 15 s profiles per side,
AnyShadowedOverlapstook 39 of 5,119 AGC worker samples on the base and 24 of 4,920 with this PR. The 16 samples that went are the list building inoverlappingand its heap allocation; what is left is the registry lock. That is about 0.3% of the worker, roughly 0.3 ms per frame.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)