Repository navigation
perf(nup): write each extracted member in place and check each folder once - #2416
Conversation
… once Unpacking a .nup spent most of its time on per-member filesystem work: every one of ~186,000 members got a .part temporary, a rename, a realpath and an lstat of every folder on its path. extract() now resolves the output once, checks or makes each folder once, refuses a link at the member, unlinks a stale file instead of truncating it, and creates the member with O_EXCL under its final name. An interrupted member is shorter than its size, so the size check writes it again. heal and unpack keep extraction_path and its temporaries. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Joey-Magnus
left a comment
There was a problem hiding this comment.
This looks right to me. I read extract() and Destination at the PR's head with the link and hard-link attacks the old .part path defended against in mind, and the new path holds up:
- The member's own name is
lstated every time (nx2/nup.py:556-561), and the file is created withO_CREAT | O_EXCL(:584). A link at that name is refused, and one that appears between the check and theopenmakes theopenfail instead of being followed. The failedopensits outside thetry, so it doesn't unlink someone else's file either. - A wrong-sized existing file is unlinked, not truncated (
:578-579), so a hard link that shares its data elsewhere keeps it.test_destination_hard_link_is_replaced_without_truncationcovers this. - On resume,
copy_toonly reads forward fromr[3]and writes sequentially (:254onward), so a killed run leaves a short file, and the existing size check writes it again. Any exception,KeyboardInterruptincluded, unlinks the half-written member (:591-595) after the out-of-space message has measured the drive. In any case,make_game.pyremoves.game(and the--checkprobe folder) before it calls--extract, so the builder never relies on resuming. - Dropping the
commonpathcheck from the hot path is safe:package_partsalready rejects empty,./.., backslash,:and device names, so arelcan't climb out of its part. healand--unpackkeepextraction_pathwith their temporaries unchanged.
The trade-off you describe in the notes, a folder checked once per run instead of before every member, seems reasonable. The old window was only narrower, and the member itself is still protected by O_EXCL.
The failed Python source checks (windows-2022, part 1/2) job is tests/test_native_astc_texture_count.py (the scanf deprecation warning under -Werror), which #2397 has since fixed on main. nx2/test_nup_paths.py and the other nup checks passed on both Windows shards and on Ubuntu. Your measurements come from synthetic packages only, which you say plainly. A timed real unpack before and after would be nice to have for the changelog, but it isn't needed to merge.
Unpacking a
.nupspends most of its time on per-member filesystem work, not on data: each of the ~186,000 members gets a.parttemporary, a rename, arealpathand anlstatof every folder on its path. Writing in place and checking each folder once makes the unpack 2.2 to 4.5 times faster on synthetic packages, about 2 minutes per unpack on a real one (estimated).What changed
nx2/nup.py,extract()only:Destinationclass: it resolves the output folder once, and checks each folder with onelstatthe first time it is reached: it creates the folder if it is missing, and refuses a link or reparse point.lstaton each member:os.open(O_WRONLY | O_CREAT | O_EXCL | O_BINARY)under its final name, so there is no.partand noos.replace.O_EXCLdoes not follow a link that appears at that name.SystemExit), the half-written member is unlinked, and the out-of-space message is built first, as before..partleft by an older builder can only sit beside the first member that still needs writing, so it is unlinked once per run.The path validation moves into
package_parts()and the link test intois_link().healandunpackstill go throughextraction_path()and keep their.part/.plaintemporaries, unchanged.Resume: no journal is needed. A member is written by sequential appends, so a run killed in the middle leaves it shorter than its listed size, and the existing size check writes it again. The builder never resumes anyway: it removes
.gamebefore every unpack and stamps only after success.Measurements
The synthetic packages were made by a script (all members
.zswith real SHA-256 digests, so the head and digest checks run in both versions). Each run is a fresh process, old (main) and new interleaved in shuffled order,extract(only="romfs")on NTFS with Defender on. Medians (min-max):On a loaded machine the gap was larger (4.5x on the zstd package). The quiet runs save about 0.7 ms per member, so roughly 2 minutes over 186,583 members. That figure is scaled from synthetic packages: I did not time a real unpack.
How it was tested
nx2/test_nup_paths.py:main. They need the symlink privilege (WinError 1314).main'snup.py.nupchecks:nx2/test_nup_out_of_space.py,test_nup_zstd_heads.pyandtest_nup_stand_in.pypass.python -I -B tests/run_all.pyon Windows gives the same failing set as untouchedmain, none new.Notes
O_EXCL..partand rename gave no stronger guarantee on NTFS.extraction_path(). This change moves the check intois_link(), so whichever lands second has a small conflict, andis_link()follows fix: security hardening passes (update offer, builder and release downloads, setup lists, shared env file, files the game reads and writes, game folders from elsewhere, HLE guest ranges) #2326's choice for OneDrive reparse points. This change keeps "the unpack refuses any".extract().