Repository navigation
Conversation
|
This pull request is labeled |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@imellag thanks, the record-time stamping itself looks sound. One question before this can go in: #9685 (c7f9eaa) made |
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the recorder changes and the new ordering test, and I agree with the concern already raised by the collaborator: PendingUnstampedWriterSerial skips every stamped note before FlushPending can apply the existing ownNotes exception, which is deliberately restricted to a single image with an existing host import. The new test covers a repeated store for one image, but not multiple flushed images or an image without a host import. Please explain why the #9685 restriction does not apply to those stamped writes, or constrain the skip to the same safe cases and add the requested multi-image/no-import regression case. I am not treating the current test as evidence for those cases.
The performance report also does not use the repository main baseline: it names a custom tree containing open macOS boot PRs and #11705. Please rerun the matched capture against current main and this exact change, and refresh the attached digest/traces as requested by the performance-report check. Finally, coordinate the same-line/test overlap with open #8887 and record the agreed ordering in Depends on per CONTRIBUTING; the overlap section currently says either PR may land second while the checklist says none.
7973cef to
156bbaa
Compare
|
@oneandonlydean @MarcZX10 thanks, you are right about #9685's restriction. A flush that stores several images, or an image without a host import, can store on the CPU or over a neighbour, and must wait for every pending GPU write over the range, stamped or not. Pushed 156bbaa (branch rebased on current
Baseline.
Why each one: #7631, #7804, #10552, #11249 libkernel/libc on macOS; #11447 and #11554 AGC driver on macOS; #10669 libSceAudioIn link. The earlier numbers in the description came from a tree that also had #11551 and #11705; they are replaced below. Same focused tests on both arms with God of War (PPSA28997), Cyclops fight, one pair on these two branches, same machine,
This is one pair only; I will add a matched trace capture of these two branches with #8887. I measured it on macOS and proposed there that it lands first; once SleglJan agrees I will set |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 156bbaa. The new guard and the no-import/multi-image regression cases address the broad skip outside the existing single-image, host-import case. I found three remaining blockers:
-
When ownStoresSkipped is true, PendingUnstampedWriterSerial ignores every overlapping note in batch.stampedNotes, not only this flush's ownNotes. NoteStampedWrites is used both for storage write-backs and UnitShadow::recordCopies. The single-image/host-import guard does not establish that every overlapping stamped note belongs to the image being flushed; a pending unit-shadow publish or another overlapping stamped writer can still be skipped. Please narrow the exception to the flush's own writes, or document the invariant that rules out unrelated stamped writers and add a regression with an unrelated stamped publish overlapping a single imported image.
-
The current heads of #8887 and #6186 both conflict with this head in shared AGC code. Pairwise merge-tree reports conflicts for #8887 in Recorder.hpp, Recorder.cpp, Texture.cpp and StorageFlushOrdering.cpp; for #6186 it reports conflicts in CMakeLists.txt, Recorder.hpp, UnitShadow.cpp and the debt file. The PR describes these overlaps but still says Depends on: none. CONTRIBUTING requires agreement on order and a declared dependency for same-line overlaps. Please coordinate and declare the agreed order. I checked #11694 as well; its current head merges cleanly with this PR, so I do not see a required ordering there.
-
The updated report includes matched traces and digests, but its baseline is e5d3b00 from a custom tree with macOS boot PRs and #11705, rather than current upstream main d13b3d7. The perf contribution rule requires a current-main/change comparison on the same machine and scene. Please provide that comparison on a supported platform or remove the performance claim and label.
The current-main merge preview for this head is clean. I am requesting these specific corrections; this is not an approval of the performance or ordering claims.
|
@MarcZX10 thanks. Updated what I can without new test runs:
|
|
Windows + NVIDIA result for head This PR's own tests: Host: Windows 11 26300, Intel i7-12700K, NVIDIA GeForce RTX 2070 SUPER (vendor 0x10de, device 0x1e84; Vulkan loader 1.4.357 from the NVIDIA driver 621.01), Khronos Validation Layers nightly 1.4.366 (b2b44c0d37) through Same focused run on
The tree also carried the open PRs #11948, #12062, #12132, #11680, #11679, #11678, #11677, #11675, #11674, #11605, #11604, #11600, all merged cleanly, so the run is a joint one. Not run on a PS5. AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
|
@MarcZX10 a preview of the combined stack you asked for, before #8887 lands. This branch is unchanged. I merged #8887's current head macOS run of that stack: Apple M4 Pro, macOS 27.2, MoltenVK 1.4.2, x86-64 Release under Rosetta 2, on our support branch
Once #8887 lands, I'll rebase onto AI-assisted: yes |
|
@imellag a data point from Ghost of Tsushima (PPSA02225), on the ownership question @MarcZX10 raised. It isn't a request to widen the skip. I cherry-picked this PR's four commits ( A local diagnostic classified each overlapping pending write in the recorder's batches whenever
In this title, nearly every refresh sync waits for stamped notes of images other than the one being flushed, and none waits for an unstamped (address-based) writer. So the single-image, own-notes exception doesn't apply to any of them. Lifting it would need the invariant the review asked for, namely that an unrelated stamped write-back or unit-shadow publish can't change what this flush keeps. This is only how one title's waits split. My diagnostic doesn't tell write-backs from unit-shadow publishes. AI-assisted: yes (Claude Code) |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 480 tests)Skipped (44)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
@milescoviello thanks for the Ghost of Tsushima data. It matches what this PR does since the review: it skips only the flushed image's own stamped notes, and only when the flush stores that single image. Your table has 0 syncs in the "own stamped notes" column, so unchanged refresh waits in GoT are what I'd expect, and the PR claims nothing for that title. The waits on other images' stamped notes are the case the invariant MarcZX10 asked for would cover. I'm not widening the skip without it. AI-assisted: yes |
|
@MarcZX10 #8887 has landed, so here is the combined-stack result you asked for. Head macOS run: Apple M4 Pro, macOS 27.2, MoltenVK 1.4.2 (Vulkan SDK 1.4.363.0), x86-64 Release under Rosetta 2.
This head isn't measured in titles yet. The GoW numbers in the description are from the earlier stack. AI-assisted: yes |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 495 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
The red AI-assisted: yes |
❌ AMD GPU test: failedPR head
Failing testsThis PR changes test files, so each failing test was also built and run on main with only the PR's test changes applied. ✅ there means the failure comes from the PR's other changes; ❌ means the test fails on main too, so this run does not show the PR caused it.
agc_driver_storage_flush_ordering: Failed (also failed when re-run alone), assertion and diagnosticsUnstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (8, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 525 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Hi imellag, Thanks for the work on this. The AMD GPU bot reported one new failure at this exact head (41f3e3c, merged onto main 7feb37e, 3 Nov, 08:27 UTC): The message it prints is: "storage flush ordering, 2 images stored again in the open batch: the second store did not wait for the earlier store, which only a single image with a host import may skip". That is the check at StorageFlushOrdering.cpp:271: the text has no "without a host import" suffix, so on that machine the test saw a host import for both images, and the second FlushPending with two images did not advance OpenSerial(). In the production change, the new flush-note recording in StorageTexture::FlushPending is limited to Could you check that case on an AMD/RADV setup (or any host where the two-image import path is taken), and either make the second store wait there or explain why the test's expectation does not hold on that driver? The bot's log is the gist linked in its comment on the PR. Separately, this branch now conflicts with main in Graphics/include/Texture.hpp. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (13 IDs in 544 tests)Skipped (48)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
… watched host imports
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 544 tests)Skipped (59)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
@oneandonlydean thanks, and thanks for the merge of I reproduced it on macOS with
So over a compared import the second store doesn't wait on macOS 27.2, Apple M4 Pro, MoltenVK 1.4.2, with
I don't have an AMD/RADV machine, so a rerun of the bot would confirm it there. AI-assisted: yes |
✅ NVIDIA GPU test: passedPR head
Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (13 IDs in 544 tests)Skipped (48)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 544 tests)Skipped (59)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Windows and NVIDIA Blackwell run, for head So this run covers the watched side of the test fix in
The 3 failures fail on
The other 12 skips also skip on No title was run. AI-assisted: yes. Claude Code ran the builds and tests and drafted this comment; the numbers are from the runs on this machine. |
|
@adam25136 thanks for the Windows / RTX 5080 run. It covers the watched side of the test fix in @MarcZX10 both open points from your last review are settled:
The description is updated to match (Depends on: none). Could you take another look? The AI-assisted: yes |
What
StorageTexture::FlushPendingsyncs when an unfinished batch writes the flushed range (#6364, 9fffcea): a writer that stamps guest memory only when its batch completes (an address-based dispatch) must have its marks land before the write-back decides which 64 KiB blocks to keep. When the writer is in the open batch, that is a fullRecorder::Sync.Some writers need no such wait: storage image write-backs (
writeBackLayers,writeBackWindows) callGuestMemory::MarkWrittenfor what they store while they are recorded, and a unit shadow publish copies bytes those write-backs already stamped. The next write-back of the same range is recorded after them in the same queue, so it lands after them on the GPU anyway. Today the flush still waits for them. Example: an image is written back at one label, written again by a dispatch, then flushed again at the next label; the second flush finds the first one's publish in the open batch and drains the queue.Recorder::NoteStampedWritesnotes such ranges likeNotePendingWritesand also keeps their write notes in the batch (stampedNotes). The two write-back paths andrecordCopiesin UnitShadow.cpp note their writes this way. The staged publish (the copy into a host buffer that writes guest memory at completion) does not.FlushPendingkeeps the stamped notes that flush recorded (the image's write-backs and its publish), fromRecorder::StampedNotesSince, in the image'sflushNotes(the last 32). A flush that stores several images keeps none, so no image takes another's notes.flushNotesto the existingownNotesexception of fix(agc): store resident linear color targets at completion points, limited to their drawn rectangles #9685, so it applies only where that one does: the flush stores a single image and that image has a host import. OtherwiseownNotesis cleared as onmain. Any other writer over the range (another image's write-back or publish, a dispatch, draw, fill, label, CPU-resolved copy, address-based lease) still makes the flush wait, and CPU readers still sync on these writes through the pending-write snapshot as before.tests/execution/StorageFlushOrdering.cppgains a stored-again case: images are stored, written again by a dispatch and flushed again in the same open batch, and the newer texels must land. With one image and a host import the second flush must not submit the open batch. It must wait with two adjacent images flushed together, without a host import, when another stamped write that is not the image's own (noted withNoteStampedWritesover the image) is in the open batch, or when the image is flushed alone after a joint flush with its neighbour. A second registration,agc_driver_storage_flush_ordering_no_import, runs the stored-again cases withAPS5_NO_HOST_IMPORT=1.mainfails the one-image case (it waits), the first head of this PR failed the two-image case, the second head failed the unrelated stamped write case and the third the alone-after-joint case (they did not wait), the current head passes all of them. The two existing cases (writer in flight, writer in the open batch) still pass.Overlaps:
FlushPendingand in this test. perf(agc): wait on a flush only for writers over the image layers it stores #8887 merged on 2026-11-02 and this branch contains it (mainmerge41f3e3c20e).recordCopieswith a write kind (ShadowPublishWrite) and conflicts with this PR inRecorder.hpp,UnitShadow.cpp, the AGCCMakeLists.txtand the AGC debt file. Order agreed with ee6aba54 on fix(agc): keep CPU stores made after a deferred DCC key store was recorded #6186 (2026-10-31): this PR first, then fix(agc): keep CPU stores made after a deferred DCC key store was recorded #6186 rebases onto it and carries the stamped flag through its write kind. This PR does not need fix(agc): keep CPU stores made after a deferred DCC key store was recorded #6186, so it is not in Depends on.writeBackWindows' imported pieces) is noted as stamped here too.Tested
No performance claim:
maincannot run titles or the AGC execution tests on macOS yet, and I have no other platform for amain-vs-change capture.Current head
a8306d56f5:41f3e3c20emergedmain88e804b0a7with #8887 (combined results in this comment),2b211c7231mergedmain5b1d381d5a, anda8306d56f5fixes the test's expectation on hosts that compare host imports instead of watching them (details). The runs below are of the earlier heads.Head
e7e78cff0e, rebased ontomain98ddd9c179. One conflict, in the AGCCMakeLists.txt:mainregistersagc_driver_registration_prepare_budgetwhere this PR registersagc_driver_storage_flush_ordering_no_import; both are kept. The other files merged without conflicts, and what this PR builds on is unchanged onmain:noteWriteandNotePendingWritesstill give each range its own write note, andPendingWriterSerialwithownNotesstill skips a write by its note. With #8887's head1f15e200f7the only conflict is inStorageFlushOrdering.cpp, where both add cases.macOS 27.2, Apple M4 Pro, MoltenVK 1.4.2 (Vulkan SDK 1.4.363.0), x86_64 Release under Rosetta. Support branch, not
main: fork branchsupport/macos-boot-1030(6747733e89) =main98ddd9c179+ only the open macOS PRs #11447 #11554 #10552 #7631 #7804 #4630 and #13220 (the duplicatewrite_watchtest registration that stopsmain's CMake on macOS), plus one local line that readsenvironthrough_NSGetEnviron()inHostReportSources.cpp(maindoesn't compile it on macOS; no open PR covers it yet). The conflicts of #11554, #10552, #7631, #7804 and #4630 withmainare resolved in the merge commits. Change: fork branchsupport/macos-boot-1030-pr12070(f426fc662b) = that branch + the four commits of this PR.Focused tests on both with
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_LOADER_DEBUG=layer,ctest -R 'storage_flush_ordering|linear_readback_label|linear_target|agc_driver_storage_(cache_budget|one_slice_alias|read_during_idle_store|write_back)|storage_cache_retained|depth_storage_reuse|flip_stores_small' -V:agc_driver_storage_flush_ordering_no_import. Bothstorage_flush_orderingregistrations pass (storage flush ordering tests passed,storage flush ordering stored-again tests passed);VK_LAYER_KHRONOS_validationas enabled byVK_INSTANCE_LAYERS, there is nowas not found but was requested by env var VK_INSTANCE_LAYERS, and noVUID-line.The 10 failures of the previous run (the "two different device resolvers" abort) are gone: #11705 has merged. Logs attached.
Previous head
3b3cd7377f(support branchsupport/macos-boot-1027,maind13b3d7c55): the same tests failed the same 10 tests on both sides for the resolver abort and passed otherwise, bothstorage_flush_orderingregistrations passed on the change, noVUID-. God of War (PPSA28997) ran through the Cyclops fight on the second head with no crash or device loss; not rerun since.python3 tools/check_conventions.py --base upstream/main(98ddd9c179): passed.tools/pr_overlap.py: conflicting lines only with #8887 (StorageFlushOrdering.cpp).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)