Repository navigation
Fix #99: deterministic multithreaded dump (record name cached during another thread's quick init) - #102
Merged
Conversation
The multithreaded `xedit dump` of `Skyrim.esm` was nondeterministic: a `QUST` name read while another worker built the record was missing its quoted `FULL`, and the official file's name caches (`mrName`, `mrShortName`, `mrDisplayName`) kept it for the rest of the dump. Two 8-thread runs of the unfixed release differed from the cached oracle at line 211,346,725 (`QNAM - Quest: QUST - Quest [0004B869] <WIGamesHideAndSeek>` against `<WIGamesHideAndSeek> "Hide and Seek"`) and line 9,024,066 (a VMAD property `Alias[001 Arvel] on QUST - Quest [00039645] <MS13>` against `<MS13> "The Golden Claw"`), and from each other. The race is `TwbMainRecord.mrInvalidateNameCache`: a record of a localized file clears its cached names when the handler generation changes, and reads the full name again only when the names are known (`mr_names_known`, upstream's `mrsQuickInitDone`). A quick init on another thread has read `FULL` but sets the flag only after the subrecords are read; a first name read on another thread in that window invalidated the cache with the flag clear, overwrote the full name with '', and the build, already past that subrecord, never wrote it again. The invalidation now leaves the full name alone while the names are not known, writes the name it reads before it publishes the generation (so a reader that sees the new generation also sees the name read with it), and keeps the old name when the elements to read it again are not there yet. A getter that gives up its wait for another thread's init (two builds need each other, `threads::init_cycles`) now answers upstream's "not yet available" text and notes the partial read, so `GetName`, `GetShortName` and `GetDisplayName` do not cache a name built from it either. Tests: `crates/xedit-session/tests/threaded_names.rs` forces both interleavings through the progress callback of a subrecord `Init` does not know (an invalidation between `FULL` and the end of the init, and two quick inits that need each other); both fail before the change and pass after it. `crates/xedit-cli/tests/threads.rs` hashes three 8-thread dumps of `Skyrim.esm` (ignored in a debug build, which is far slower). Verification: three 8-thread dumps and one 4-thread dump of `Skyrim.esm` are byte-identical to each other and to the cached 4.1.5q oracle (8,322,131,525 bytes, `cmp`); `cargo xtask parity dump --game sse --file Skyrim.esm --jobs 1` is `equal` three times; `crates/xedit-session`'s `threaded_names` 2 of 2; `cargo clippy --workspace --all-targets -- -D warnings` and `cargo xtask check` exit 0; `cargo test --workspace --no-fail-fast` is green apart from the known `copies_a_skyrim_npc_as_an_override` failure, which fails identically without this change. Co-Authored-By: Claude Code <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
TwbMainRecord.mrInvalidateNameCacheruns when a localized file's handler generation changes: it clears the cached names and reads the full name again only whenmr_names_known(upstream'smrsQuickInitDone) says the names are final. A quick init on another worker has readFULLat that point but sets the flag only after the subrecords are read. A first name read on a second thread inside that window invalidated the cache with the flag still clear, overwrote the full name with'', and the build, already past theFULLsubrecord, never wrote it again — so the record kept a name without itsFULLfor the rest of the dump, inmrName,mrShortNameandmrDisplayName(the official file's caches). The invalidation now leaves the full name alone while the record's names are not known, publishes the generation only after the names it read (a reader that sees the new generation sees the name read with it), and keeps the old name when the elements to read it again are not built yet.A getter that gives up its wait for another thread's init (two builds need each other,
threads::init_cycles) now answers upstream'snot yet availabletext and notes the partial read, soGetName,GetShortNameandGetDisplayNamedo not cache a name built from it either.Before (release at main
6d9646d,--threads 8, two runs ofSkyrim.esm)Both runs differed from the cached 4.1.5q oracle, and from each other. Run 1:
char 8161633134, line 211346725, where the port wroteQNAM - Quest: QUST - Quest [0004B869] <WIGamesHideAndSeek>and the oracle has<WIGamesHideAndSeek> "Hide and Seek". Run 2:char 411102402, line 9024066, where the port wroteAlias[001 Arvel] on QUST - Quest [00039645] <MS13>and the oracle has<MS13> "The Golden Claw"(the issue's own evidence was the same class at lines 210,730,161 and 581,277).After (release with this branch)
--threads 8and one at--threads 4: byte-identical to each other and to the cached oracle, 8,322,131,525 bytes (cmp).cargo xtask parity dump --game sse --file Skyrim.esm --jobs 1:equalin three consecutive runs (1 of 1 equal, port peak 2.0 GiB).Tests and gates
crates/xedit-session/tests/threaded_names.rsforces both interleavings through the progress callback of a subrecord the definition does not know: an invalidation between theFULLread and the end of the init, and two quick inits that need each other. Both tests fail before the change (the first leavesCLAS - Class [00000800] <TestClass>and the second caches it; the failing assertions were checked by stashing the core change) and pass after it.crates/xedit-cli/tests/threads.rshashes three 8-thread dumps ofSkyrim.esmand requires them equal; it is ignored in a debug build (an 8.3 GB dump is a release-scale run) and passed in release (217 s).cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warningsandcargo xtask check: exit 0.cargo test --workspace --no-fail-fast: every target green except the knowncopies_a_skyrim_npc_as_an_overridefailure, which fails identically on the unmodified tree.Closes #99.