Repository navigation
Order only GetCompiled's lookup in the import wrapper cache - #230
Merged
Merged
Conversation
91ec2b6 ("Order the import wrapper cache's compile path, not its mutex") took the cache's ordered lock for all of GetCompiled, but on a miss GetCompiled goes on to CacheCompiledWrapper, whose ModificationScope takes the same ordered lock again. Recording holds an ordered lock through a recursive mutex, so that went unnoticed there; replaying holds it through a non-recursive ReplayLock, and the nested acquisition dies on "Assertion failed!" at linker/ReplayLock.cpp:117. Every replay of a program that tiers a wasm-to-JS import wrapper up (Runtime_TierUpWasmToJSWrapper) reaches it: crash bucket Other/v8::internal::wasm::WasmWrapperCache<ImportWrapperCacheKey> on linux-node-20261111-5f07c830593a-d18bfb5f323d-dev. The ordered lock now covers just the entry_map_ lookup under mutex_, which is what the ordered mutex this replaced covered. Compiling stays outside it, as upstream has it, and publishing is ordered by ModificationScope, which also settles which of two racing compiles wins. Recreated by backend's node-recording case webassembly/import-wrapper-tier-up, which fails on that build and passes with this change.
A JS import starts out calling through the generic wasm-to-JS wrapper, which counts the import's calls and, once its budget is used up, calls Runtime_TierUpWasmToJSWrapper to compile a wrapper. That wrapper is published through the cache handle's code pointer, which every instance with the same CacheKey calls through, in every isolate. So when workers share an import's key, the first one to tier up switches the others over at a point decided by cross-thread timing, and whether another worker used up its own budget first, and so calls the runtime and takes the cache's ordered lock, differs between recording and replaying. The replay dies on a LinkerMismatch, either reaching GetCompiled where the recording didn't or skipping it. When recording or replaying, BuiltinForWrapper no longer hands out the generic wrapper, so Get compiles the wrapper when the import is resolved and nothing tiers up later. Get's lookup now takes the ordered lock, because whether it finds another isolate's wrapper decides whether it compiles one; publishing was already ordered by ModificationScope. Ordering Get's lookup alone doesn't help: the backend's node-recording case webassembly/import-wrapper-shared-tier-up still failed 8 of 8 runs with it. The case fails on the previous commit and passes with this one.
The cache's entries are weak: an entry expires when its last instance goes away, which happens when another isolate is torn down or collects it. So with workers sharing an import's key, whether a worker's Get finds the wave's wrapper or compiles its own still depended on when an earlier worker exited, and the replay could compile, and take the cache's ordered lock, where the recording didn't. When recording or replaying, Get now keeps every handle it creates for the rest of the process. With wrappers compiled up front, Get creates every entry, so none of them expire. They live in a file-level vector rather than in the cache because the cache must stay the size of WasmWrapperCache. With the previous commit alone, the backend's node-recording case webassembly/import-wrapper-shared-tier-up still failed 8 of 8 runs, on a mismatch in Get at instantiation; with workers held alive until the whole wave was done it passed 6 of 6. With this commit it passes 8 of 8, as do the other webassembly cases.
The wrappers Get keeps alive when recording or replaying were never released, so their code-pointer-table entries stayed allocated and WasmCodePointerTable::TearDown's DCHECK that the freelist is empty would fail at exit in a debug build. The comment also claimed the cache is never freed, but GlobalTearDown deletes it. ~WasmEngine now releases them through WasmImportWrapperCache:: RecordReplayTearDown, next to NativeModuleCache::RecordReplayTearDown, while the engine can still take their code: each wrapper then goes the way upstream sends it when its last instance dies. The comments now say what is kept, the limits of that (one wrapper per CacheKey for the process's life, the wasm-to-C-API entries GetCompiled adds aren't kept), and that Get, not BuiltinForWrapper, compiles the wrapper. Verified on a release build only: recording the backend's node-recording case webassembly/import-wrapper-shared-tier-up under gdb reaches RecordReplayTearDown with a non-empty vector before the code pointer table's TearDown, and the process exits normally. The webassembly cases pass. The debug DCHECK itself was not run.
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.
91ec2b6 ("Order the import wrapper cache's compile path, not its mutex") took the cache's ordered lock for all of GetCompiled, but on a miss GetCompiled goes on to CacheCompiledWrapper, whose ModificationScope takes the same ordered lock again. Recording holds an ordered lock through a recursive mutex, so that went unnoticed there; replaying holds it through a non-recursive ReplayLock, and the nested acquisition dies on "Assertion failed!" at linker/ReplayLock.cpp:117. Every replay of a program that tiers a wasm-to-JS import wrapper up (Runtime_TierUpWasmToJSWrapper) reaches it: crash bucket
Other/v8::internal::wasm::WasmWrapperCache on linux-node-20261111-5f07c830593a-d18bfb5f323d-dev.
The ordered lock now covers just the entry_map_ lookup under mutex_, which is what the ordered mutex this replaced covered. Compiling stays outside it, as upstream has it, and publishing is ordered by ModificationScope, which also settles which of two racing compiles wins.
Recreated by backend's node-recording case
webassembly/import-wrapper-tier-up, which fails on that build and passes with this change.