Repository navigation
Conversation
3735a0f to
096de9a
Compare
|
#2684 addresses a complementary cause of cache growth: translator lifetime. This PR changes how cache bytes are read and published, without deciding when a cache becomes stale. The integration point is I’ll keep this PR scoped to byte storage; there is no reason to duplicate #2684’s invalidation policy here. This is a source-level overlap assessment, not a combined-branch runtime validation. |
Keep warm cache data in owned file-backed storage across base and play cache reads, merges and saves. Validate headers before bulk acquisition and publish only successful driver copies at their actual length. Preserve old generations on I/O or cache-creation failure, and report retryable temporary cleanup errors. Cover native ownership, syscall refusals, producer results and semantic mutants on Windows and Linux. Document measured preparation and checkpoint reloads with their platform, driver and mixed-host limits. Refs NotRexed#2623 Assisted-by: OpenAI Codex
Label the three matching782 game processes, forced base publication and successful fresh-process consumption separately from the later rebase/build. Preserve the earlier measured evidence and its limitations. Assisted-by: OpenAI Codex
096de9a to
23c5008
Compare
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, this is a careful piece of work: unique temporaries, header check before the payload, flush/fsync before the rename, the old file and pcache_saved_size left alone on any failure, and the base mapping dropped (data = {}) before play_files_merge can try to replace the same file on Windows. The fault-injection test is a nice touch. CI is green on 096de9a5 (including Windows part 2/2 and test_pcache_header_read, which you've adapted to count ReadFile/pread instead of fread). GitHub reports conflicts with main in docs/development.md and docs/tested-configurations.md, so it needs a rebase.
A few things before this can go in:
-
Linux save on a full disk crashes instead of failing.
writable()sizes the temporary withftruncate, which leaves a sparse file on ext4/btrfs/xfs, and then the driver writes into the mapping. If the disk fills up, that becomes a SIGBUS insidevkGetPipelineCacheData(often at exit, in the forced save), where the oldfwritejust returned short. Doingposix_fallocatebeforemap(true)turns it back into a clean failure. Windows is fine here, becauseSetEndOfFileon NTFS allocates up front. The new doc says "reserves the queried capacity", which is only true on Windows today. -
Orphaned temporaries pile up. The old code always reused one
.tmpname, so a save killed partway (closing the game during the exit save, a crash, power loss) left at most one stray file, and it got overwritten next time. With.tmp-<pid>-<n>/.tmp-XXXXXX, every interrupted save leaves a new file of up to the full cache size (GBs) in the game folder, and nothing ever deletes them. Couldpcache_initsweepmkw_vk_pipelines*.bin.tmp-*siblings? An age check would cover a second instance that's still writing. -
The Linux read path copies the whole cache to disk on every launch. For a 1-2.5 GB base cache, that's a full extra write of the file at every start (plus each play file), before the driver sees a byte. That costs startup time on slow disks and wears SSDs, and it fails outright when free space is below the cache size. Our own writers never truncate in place; they always rename a new inode over the old one. A
MAP_PRIVATEmapping of the opened fd already keeps its inode alive across that rename, so the snapshot only protects against an outside process truncating the file. That's a narrow case for a GB of I/O per launch. I'd map the source directly on Linux, like Windows does. If you want to keep the snapshot, please measure it on Linux (startup time and bytes written, on a large cache) first. -
Measurements (our rule for perf PRs: before/after on the same scene, with the hardware named). The Windows pair is there: 9,693,954,048 vs 9,056,952,320 peak private bytes, same 487 MB input cache, warm, RTX 5070 Ti / 9800X3D. Thanks. Two asks:
- "Private bytes" excludes mapped file pages by definition, so it drops whenever a buffer moves to a mapping. Peak working set (or commit) for the same pair would show whether physical memory actually went down.
- Linux has no run at all, and it's the platform with the different (snapshot) path. A Linux before/after on the same scene (peak RSS and startup time) is needed before this merges.
-
Docs. The worklist row is separated from the table by a blank line, so it won't render as part of the table (inline). Eight tested-configurations rows for one PR is a lot, and most read as a validation log ("bridge PNG", "protected checkpoint hashes"). The table is for what runs on which hardware. Please fold them into one or two rows: the A/B preparation pair, and the checkpoint load/save. The detail fits better in the PR description.
Smaller points are inline.
| LARGE_INTEGER length{}; length.QuadPart = (LONGLONG)n; | ||
| return SetFilePointerEx(file_, length, nullptr, FILE_BEGIN) && SetEndOfFile(file_); | ||
| #else | ||
| return ftruncate(file_, (off_t)n) == 0; |
There was a problem hiding this comment.
This leaves holes on Linux filesystems, so the mapped writes that follow can SIGBUS on ENOSPC (point 1). posix_fallocate(file_, 0, n) (it returns an error code, not -1/errno) before mapping avoids it.
| return false; | ||
| #else | ||
| if (path.empty() || path.find('\0') != std::string::npos) return false; | ||
| std::string name = path + ".tmp-XXXXXX"; |
There was a problem hiding this comment.
Each interrupted save now leaves a uniquely named, cache-sized file that nothing removes (point 2). Also a nit: mkstemp creates the file 0600 and rename keeps that mode, so the published cache goes from umask-based (0644) to 0600. Probably harmless, but an fchmod to 0644 would keep the old behaviour.
| if (result.file_ == INVALID_HANDLE_VALUE) return {}; | ||
| BY_HANDLE_FILE_INFORMATION info{}; LARGE_INTEGER n{}; DWORD got = 0; | ||
| if (GetFileType(result.file_) != FILE_TYPE_DISK || !GetFileInformationByHandle(result.file_, &info) || | ||
| (info.dwFileAttributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT)) || |
There was a problem hiding this comment.
Refusing reparse points (and O_NOFOLLOW on Linux) is new behaviour: the old fopen followed them. Files in a OneDrive-synced folder carry FILE_ATTRIBUTE_REPARSE_POINT (cloud-file tags), and some Linux users symlink caches to another drive. Either way, the cache would then silently never load. Is there a reason to reject them? If not, I'd drop the check, or limit it to directories.
| length = (uint64_t)st.st_size; | ||
| if (length > SIZE_MAX) return {}; | ||
| if (!valid(header, length)) return rejected_file(); | ||
| if (!result.create_temporary(path)) return {}; |
There was a problem hiding this comment.
This is the full-size disk copy per read (point 3). Mapping source directly with MAP_PRIVATE survives our rename-based replacement, because the inode stays alive.
| if (part.rejected()) remove(path.c_str()); // Only a proven incompatible header can discard a generation. | ||
| continue; | ||
| } | ||
| if (vkCreatePipelineCache(dev.device, &ci, nullptr, &tmp) != VK_SUCCESS) continue; |
There was a problem hiding this comment.
Nit/behaviour change: a play file whose header matches but which the driver refuses used to be removed. Now it's kept and re-read at every start. It never counts toward merged, so compaction never removes it either. Removing it here seems fine. An I/O failure (part.empty() && !part.rejected()) is the case worth keeping, and you already do.
| | 0.11 | Crash at the door after the owl intro (Navi wakes Link, 2.0.13): access violation in `igvk64.dll` at start-up; GPU: Intel UHD Graphics (i3-1115G4, 128 MB shared, no mesh shaders: the best-effort fallback, #1001); reach is Intel's 3.5% | S0 | 0.035 | 0.4 | | #bug-reporting "2.0.13 Crash after you see the owl during intro cutscene" (5 logs) | | ||
| | 0.04 | FSR 4 ticked in the Builder after the game was built does not rebuild it (the Builder says up to date); FSR 4 DLLs added by hand get no AMD license prompt, as expected. Open: #2591 | S3 | 0.10 (RX 9000) | 0.4 | | #2511; #bug-reporting "Not Getting AMD License Agreement Message" | | ||
|
|
||
| | 0.01 (provisional) | Pipeline cache persistence holds a complete temporary heap copy on top of the driver cache. Preserve the warm cache with file-backed storage; measured preparation memory, no crash/FPS fix claimed. Reach is an unmeasured 1% planning assumption for large-cache installations | S3 | 0.01 | 1.0 | Arkex-ai | #2623; [claim](https://discord.com/channels/1568810484818378804/1570103281856806962/1570255465806565520); native Windows/Linux file tests and Windows preparation/checkpoint reload | |
There was a problem hiding this comment.
The blank line above (54) ends the table, so this row renders as a stray line instead of a table row. It should go directly under the 0.04 row, in score order.
|
Your push to 23c5008 landed while I was reviewing. It rebases on main and adds one tested-configurations row, and the code in |
|
I’m addressing the remaining storage review before rerunning the measurements. The Linux capacity-reservation guard is implemented locally: the ENOSPC fault test fails against Next are interrupted-save cleanup, Linux source mapping, symlink/cloud-file compatibility, rejected play-file cleanup and the documentation layout. I’ll keep the PR draft while closing those points and collecting repeated Windows working-set/commit measurements plus the required Linux RSS/startup comparison. One separate CI issue: AppImage fails before compilation in System packages ( |
Reserve Linux mapped output before driver writes and map input files directly. Collect aged cache temporaries without deleting active writers, preserve regular link/cloud reads and existing Linux file modes, and discard data the driver rejects while retaining I/O failures. Keep the native fault and cleanup coverage and fold the hardware notes into two concise rows. Assisted-by: OpenAI Codex
|
Test request T-U8LS: pass Hardware: RTX 3080 10 GB, NVIDIA 621.14, i9-12900K, 64 GB, Windows 11. Built 23c5008 (the PR head, What I checked: the repeated same-scene before/after memory pairs the description still asks for, with a large real cache. Before every run, the build's cache files were replaced by one fixed copy of a prepared install's cache:
Result:
Round trip: after the last PR run, a further PR start without restoring the copy read the cache that run had published ( Log lines (pair 1, the same in all six runs apart from the save sizes): Not tested: frame rate, start-up time, Linux, AMD and Intel, a cache-save failure, and the start compaction at Tested-configurations row, if you want to fold it into the PR:
|
|
Tested on
Why steady-state field peak is unchanged while full-save peak drops by the cache size:
|
Replace the single preparation pair with the current eight-process game cohort, including A/A variability and separate private/working-set results. Attribute the RTX 3080 report to its tester and earlier tested commit. Assisted-by: OpenAI Codex
|
Thanks both — the distinction between cache-I/O peaks and steady gameplay memory is useful. The review fixes are now in I also finished eight fresh Windows game runs on that implementation (A/A, then AB/BA/AB), with identical restored inputs and the same Kokiri checkpoint. The three pairs showed 925/1,064/900 MiB lower peak private bytes, but A/A itself varied by 980 MiB. Working-set and steady-private results do not establish a consistent reduction, so the description now states that limit explicitly. All processes quit normally and preserved the checkpoint. I've attributed the RTX 3080 result to |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for working through all of it. I re-read 84a006d5 and b0cc8bdb against my review at 096de9a5. Every code point is fixed, and the docs are cleaned up. What's left is the Linux measurement.
Fixed:
- Full disk on Linux.
posix_fallocatenow runs beforemap(true), and a failure goes throughreset(), which unlinks the temporary and leaves the old cache andpcache_saved_sizealone.- On filesystems that don't support fallocate, glibc writes one byte per block instead. That's slow on a GB cache over NFS, but it still reserves the space and fails cleanly. tmpfs, btrfs, xfs and extent-based ext4 support fallocate directly. An
EINTRjust fails that save, and the next one retries. - The doc's "clean failure, not SIGBUS" holds everywhere except copy-on-write filesystems like ZFS, which can't truly reserve space. That's not worth more code.
- On filesystems that don't support fallocate, glibc writes one byte per block instead. That's slow on a GB cache over NFS, but it still reserves the space and fails cleanly. tmpfs, btrfs, xfs and extent-based ext4 support fallocate directly. An
- Orphaned temporaries. The sweep looks safe to me:
- It only touches the two temp name patterns, owned by us, with one link, not a symlink, and 24 h old.
- A live writer is protected twice. On Linux the writer holds
flockfor as long as its fd is open. On Windows the writer's handle shares nothing, and the sweeper opens the file withFILE_SHARE_DELETEonly. refresh_modified()before the close covers the gap between close and rename.- The inode re-check before
unlinkcloses the path-swap race. - The one leftover race: if the sweeper takes the lock between another instance's
mkstempandflock, that save fails and the next one retries. It never deletes anything. - Nit, optional: since the lock already protects writers, the 24 h age could come down to an hour or so. A crash during the exit save then wouldn't leave a cache-sized file for a whole day.
- Linux read. It now maps the source directly with
MAP_PRIVATEand writes no temporary.- The only
preadleft is the 32-byte header. A short read there just makes that start's read empty, and the file is kept, which is the right outcome. - Files over 4 GB are fine: the length is 64-bit throughout, and both mmap and
MapViewOfFiletake the full size.
- The only
- Smaller points:
- Symlinks and OneDrive placeholders now open as
fopendid. - A play file the driver refuses is removed after
reset()releases the view. - Existing caches keep their mode. A new file stays 0600, which is fine by me.
- Symlinks and OneDrive placeholders now open as
- Docs. The worklist row is back in the table, in score order. The tested-configurations rows are folded into three. Thanks.
Evidence so far:
- dankman42's RTX 3080 run (Windows, at
23c50087) is the clean result: three interleaved pairs with a 1.6 GB cache and three full saves per run. Median peak private bytes drop by about 1.4 GB, every PR run is below every main run, the working set is level as expected, and a fresh start reads the saved cache. Since then only the I/O code has changed, and the save path is the same apart from the reservation. - Your own pairs agree in direction, but your A/A spread of 980 MiB is as large as the effect, so I'd lean on dankman42's numbers.
- Brett-D's RTX 5090 run with no full save shows no change, which fits the scope the description now states.
What's needed to merge: a Linux before/after on the same scene, as the description already says.
- Please take peak
RssAnon(orVmHWMtogether withRssAnonfrom/proc/<pid>/status) and the time to thevk: pipeline cache ... KBline, with a large cache and at least one full save, and name the hardware. - Plain RSS counts the mapped file pages, so it will look level, the same way the working set does on Windows.
- We have a Linux AMD run of this PR going for the T-U8LS request. If it reports those numbers, that covers it. Once the Linux run is in, take it out of draft and I'll approve.
CI and conflicts: all green on b0cc8bdb, including AppImage and Windows part 2/2. It merges cleanly with main.
Order with #2625: I said earlier that #2625 should land first. #2625 hasn't moved since its review, and its fold-never-fires problem is still open, so I wouldn't hold this PR for it any more. Whichever is ready first goes in.
- The conflict is small: adjacent hunks in
play_files_mergeand the shared test file. - After the rebase, #2625's session file is written through
PipelineCacheFile::writable. An interrupted rewrite leavesmkw_vk_pipelines_play_<n>.bin.tmp-XXXXXX, which matchestemporary_name(). The running session's own temporary is locked, so the sweep covers it as is. - #2684 and #2456 also touch
pcache_initand the save path, so expect small textual conflicts there too. Your note on where #2684's stale check goes still applies.
Summary
Large Vulkan pipeline-cache reads and full saves allocate a cache-sized heap buffer. This replaces that temporary allocation with file-backed storage. The benefit targets cache I/O peaks; it does not promise lower steady gameplay memory or higher FPS. Fixes #2623.
Technical changes
MAP_PRIVATE; atomic replacement preserves the inode. Regular symlink/cloud-file reads use normal filesystem resolution.Validation
Implementation tested:
84a006d5773bd7b20f3f94ffc3a73d7cf23150e8, based on51da0464. Current headb0cc8bdb6a4b97bd9431662468e94775f195e197only updates tested configurations; no source/build input changed.84a006d5: source/SPIR-V/lint, Windows backend/Builder, AppImage. All applicable CI also passed on documentation headb0cc8bdb: source checks, Windows, AppImage; release-asset publication is skipped for the PR.Still draft: actual Linux game RSS/startup remains unvalidated. OneDrive integration has not been exercised. External in-place truncation is unsupported; mapped-media faults are not recoverable C++ I/O exceptions.
Memory results and external reports
RTX 5070 Ti 16 GB, NVIDIA 621.01; Ryzen 7 9800X3D / 32 GB; Windows 11, 1280x720, 60 fps cap, FG off, precompile/prediction off,
RX_VK_PLAY_CACHE=0. One fixed 1,124,480,456-byte base cache plus one play file and warmed translator inputs were restored before every process. Driver cache stayed warm. Sampling started within 0.5 s, every 100 ms, with cumulative OS working-set/commit peak counters.The A/A private peak differed by 979.9 MiB, a material spread. These bounded observations support investigating cache-I/O commit peaks, not a precise universal saving. No consistent steady private/working-set reduction, physical-memory, startup-time or FPS gain is established. Page-fault counts do not distinguish hard faults.
dankman42's RTX 3080 report compares earlier
23c50087against51da0464: three interleaved pairs with a 1.6 GB cache and three full saves per run, about 1.4 GB lower median peak private bytes, level working set, and a successful saved-cache reread. This is attributed earlier-head evidence.Brett-D's RTX 5090 report finds identical steady-state memory/FPS without a full save, consistent with the scoped cache-I/O benefit; the exact PR commit is not specified. Neither external report validates Linux or the current review changes.
Warm-up runs that replaced stale translator inputs are excluded. Coverage is early Kokiri only; no campaign, Linux/AMD/Intel or OneDrive claim. Earlier
7820853dalso passed the separate checkpoint-load, forced-save and fresh-read sequence; it is historical correctness evidence.