Repository navigation
fix(agc): compare Windows host imports after late driver writes - #1441
Conversation
|
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.
|
|
Ran this on Windows, which the description says couldn't be run: Windows 10 (19045), RTX 3060 Laptop (driver 616.92), MinGW-w64 GCC 15.2.0, Release,
For comparison, I tried keeping imports watched instead: popcoder565/AnyPS5@fix/agc-import-pin-marking. The driver's second marking covers every page of the import about 1-2 ms after the window, so
I haven't opened it as a PR, since this one already targets #1434. Whichever approach the maintainers prefer is fine. |
bab05aa to
da71dac
Compare
da71dac to
2b2636a
Compare
|
Independent Windows retest of the fresh-private-backing follow-up. Tested PR head These results cover the two pinned revisions above. Newer main, rebased revisions and the eventual merged tree have not been tested. Windows 11 Pro (26300), desktop RTX 3060, driver 617.42; WinLibs MinGW-w64 GCC 15.2.0 UCRT/posix/SEH, Ninja Release, BUILD_TESTING=ON, SPIRV-Tools=OFF. Vulkan was required, the physical RTX 3060 was selected, and shader caching was disabled. No import-policy override, artificial delay, game input or driver change. Normal Windows host results, with workspace-local TEMP/TMP:
The PR's default build, patched libs target, local conventions check and diff whitespace check passed without a local source correction. The Windows private-address-reuse case executed in all 30 write-tracking runs, including protection-only exclusion, fresh-backing restoration, stale-generation invalidation, a later CPU edit and re-import. The recorder is still unregistered in CTest, so it was explicitly built and run. No Main's intermittent GPU failure did not reproduce here. These results therefore corroborate the latest Windows build, fresh-backing integration and recorder follow-up, but do not establish a before/after driver fix on this configuration. A rerun on the original Windows 10 / RTX 3060 Laptop / 616.92 configuration remains useful. No performance or game-compatibility conclusion is claimed. The initial restricted AppContainer full run had 16 failures; its disk-cache write failure also occurred on main and disappeared with workspace TEMP/TMP. All 420 passed in the normal host run. Initial restricted failures are retained locally and are not claimed as PR regressions. AI-assisted: OpenAI Codex assisted with preparation, execution and analysis of this validation and report. No production code change was authored. |
|
Independent Windows retest of the fresh-private-backing follow-up. Tested PR head These results cover those pinned revisions. Newer revisions and the eventual merged tree have not been tested. Windows 11 Enterprise (10.0.26200), NVIDIA GeForce RTX 3050 Ti Laptop GPU, driver 591.55; WinLibs MinGW-w64 GCC 15.2.0 UCRT/posix/SEH r2, Ninja Release, The targeted execution tests selected NVIDIA, required Vulkan, and disabled shader caching. No import-policy override, artificial delay, game input, or driver change was used. Dedicated validation-harness results, excluding additional CTest runs:
The PR’s full build, patched The recorder remains unregistered in CTest, so it was explicitly built and run. No Main’s intermittent GPU failure did not reproduce here. These results therefore support the latest Windows build, fresh-backing integration, and recorder follow-up, but do not establish a before/after driver fix on this configuration. A rerun on the original Windows 10 / RTX 3060 Laptop / 616.92 configuration remains useful. No performance or game-compatibility conclusion is claimed. Initial sandbox build attempts encountered Git ownership/dependency-check problems and a stalled Ninja process. The build completed after resuming outside the sandbox, and the full CTest run passed in the normal Windows host environment. Those build-environment problems are not claimed as PR regressions. AI-assisted: OpenAI Codex assisted with preparation, execution, and analysis of this validation and report. No production code change was authored. |
It records a behaviour gap, not a silent stub, and sat at the end of its section. Co-Authored-By: Claude <noreply@anthropic.com>
|
Checked on Linux: build and full ctest (459/459) on top of current Added one commit of ours:
Notes, not blocking:
Merging once CI is green. |
2b2636a to
fd70266
Compare
|
I ran Ghost of Yōtei (PPSA26344 v1.008) on plain Yōtei creates its Vulkan device twice. On What plain DetailsWindows 11, RTX 3080 Laptop, the same post-gamma save, With the revert: 284 direct storage uploads (2,594 MiB) per 10 s on average against 1,659 (19,676 MiB), and 0.98 against 0.21 fps over the last 100 s. The revert run came second and started with the 104 shader cache entries the first run wrote (27 for the first); both created 21 pipelines. Our tree (integration on main 4b005b2): The run before this PR mirrored the same range and later released the mirror for an import. With the revert, a 1,200 s capture pass and a 3,600 s run in our tree had no FATAL and 0 unwatched heaps. |
…ettle boykopovar#1441 compares every Windows host import because a driver can mark imported pages written again shortly after the import window (1.2-2 ms on the NVIDIA driver in boykopovar#1434), and those marks dropped GPU results. The comparisons make PPSA22520 spend 6.8 s of every 10 s revalidating resources: about 7.5 FPS instead of 59. Imports stay watched. Each import keeps a copy of its bytes for a settle period (1 s, APS5_IMPORT_SETTLE_MS): a page marked in that period whose bytes still equal the copy is stamped maybe written, like the import window's own marks, and a page whose bytes changed is a CPU store. One walk of the import at the next collect after the period applies the same rule to the rest. An import whose copy would exceed 256 MiB, or 512 MiB with the copies already kept, is compared, as is every import under APS5_IMPORT_SETTLE_MS=0 or APS5_WRITE_WATCH_IMPORTS=unwatch, which is now read on Windows too; =watch keeps plain watching. Unwatching a range or giving it fresh private backing drops its copy. On Windows the DCC first-write test now rewrites the imported block with its own bytes between the import and the write-back, so it reproduces boykopovar#1434 on every driver: it fails in every run with plain watching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On Windows this change costs PPSA22520 (Animal Well) most of its frame rate: about 59 FPS before it, 7.5 FPS after (Windows 11, RTX 4070 SUPER). A #1847 keeps imports watched and tells the late driver marks apart from CPU stores by comparing a copy of each import's bytes for a settle period, so #1434 stays fixed. @ToyuAU @adriadam10, a review would be welcome. |
|
Regression report for PPSA12544 (Hollow Knight: Silksong) on Windows after this change landed ( Setup: Windows 11, MinGW GCC 15.2 Release, NVIDIA RTX 3080 (driver reports Vulkan 1.4), Before (host imports stay write-watched): the title reaches its main menu at 59.94 FPS (window-title counter, 2848 frames at the 60 s mark). The player also played past the menu into gameplay on the pre-#1441 code at well over 30 FPS (reported by the user; I only measured the menu). After (host imports compared, as merged): the same menu runs at 9.9 to 10.3 FPS (655 frames at 66 s, 733 at 75 s, warm shader cache).
Experiment: I locally restored the previous behaviour on Windows with an environment switch in So the compare path is correct for the late-driver-write case in #1434, but on this GPU it makes every Silksong frame re-upload its render targets. Two questions for the author and maintainers:
I can test any change on this title and report the numbers. I have not changed upstream code for this. AI assistance: this report was prepared with Claude Code; all numbers above are from my own runs. |
|
@ps54all @Celegans12 my PR #1847 does what you're both describing: imports stay write-watched on Windows, and only the driver's late marks right after an import are checked against a copy of the import's bytes, so #1434 stays fixed. |
|
Follow-up: #1847 was closed, and #2096 (merged 10-09) fixes this for PPSA12544. Same machine (Windows 11, RTX 3080), Written with AI assistance. |
What
Addresses #1434: Windows drivers can mark imported pages dirty after the synchronous import window. Treating those marks as CPU stores makes storage-image write-back discard newer GPU results. Windows host-pointer imports use the existing byte-comparison paths instead.
GuestMemory::Unwatchexcludes overlapping 64 KiB tracker blocks and invalidates collection memos. A new private allocation that fully replaces an excluded block restores tracking, stamps a new generation, removes old driver-store pieces and invalidates collection memos. Protection-only commits, shared remappings and partly replaced blocks retain the exclusion. A later import excludes the replacement again.WindowsMappings::Commitreports only the private allocations it creates from placeholders, coalescing adjacent fresh spans.GuestArenanotifies the tracker after the mapping mutex is released, avoiding the inverse lock order of write collection. Commit generations are recorded with exclusions so a delayed notification cannot re-enable tracking for an import made after that commit. Generation records are stored sparsely for excluded blocks. The VirtualAlloc2 documentation distinguishes placeholder replacement with a private allocation from committing already committed memory.This addresses the address-reuse failure reported in the recorder executable without weakening
AllocateWatched's assertion. The fresh-backing observer and Windows integration test still need a Windows build and hardware retest; this remains a draft.The test target now links
libc, configures Windows unwind and supplies the Windows runtime library path, correcting the reported MinGW__gxx_personality_v0link failure.Related: #1046 changes the Linux probe and includes the same recorder snapshot-reuse guard. #1310 overlaps the write-tracking tests. Neither is a dependency. The Linux probe is unchanged here. The alternative linked in the Windows report keeps imports watched and classifies an all-pages dirty sweep; this PR continues using byte comparisons instead of that heuristic.
TechnicalDebt records that snapshot reuse and unit shadows remain unavailable on excluded backing, that only complete private-backing replacement restores coverage, and that restoration on import retirement and comparison performance remain future work.
Tested
macOS arm64, AppleClang 17, C++20, rebased onto main
6b573862:agc_write_watch_coveragepasses with-Wall -Wextra -Werrorand AddressSanitizer/UndefinedBehaviorSanitizer. It covers exclusions, fresh-backing restoration, partial replacements, clipping, adjacent blocks, repeated imports and stale/same-generation mapping notifications.python3 tools/check_conventions.py --base origin/mainandgit diff origin/main...HEAD --checkpass.tools/tests/test_progress.pypassed. These are not full Windows build results.A new Windows-only
agc_write_trackingcase uses the real arena and tracker: a protection-only commit retains the exclusion; resetting/releasing and allocating the same address with fresh private backing restores it, invalidates old stamps, detects a subsequent CPU edit and excludes a second import again. This case was not run locally.Windows results reported by popcoder565, for main
bc1fbe81merged with the originalbab05aafand the suggested two linker lines: Windows 10 (19045), RTX 3060 Laptop, driver 616.92, MinGW-w64 GCC 15.2.0, Release.agc_driver_dcc_keys_first_writeandagc_driver_srgb8_color_target: 20/20 each, versus approximately one-third and three-quarters failures on main.agc_write_watch_coverageandagc_write_tracking: pass.agc_driver_recorder_testsexecutable failed 3/3 atAllocateWatched; main passed 10/10. Those observations predate this PR's fresh-backing follow-up and do not establish that the latest revision fixes the hardware regression.Draft pending a supported Windows rebuild and reruns of
agc_write_tracking, the two original GPU regressions, full CTest and the separately builtagc_driver_recorder_testsexecutable. No full Linux build or title run is claimed locally.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