Make __instance_signal and __instance_terminated fully indepedent #6270
Conversation
|
The old code had I've done a thorough review of all the changes. The code is well-structured, safe, and properly tested. I don't see any actionable issues — no logic bugs, no safety issues, no missing compat flags (since it's gated by an existing autogate), and no backward compatibility concerns. LGTM |
Merging this PR will not alter performance
Comparing 
Footnotes
|
54846e2 to
634b63a
Compare
guybedford
left a comment
There was a problem hiding this comment.
To update on the potential memory concerns, I created a negative test in Edgeworker today which disabled the GC preamble and verified WebAssembly.Instance finalization in a GC loop. I was able to verify that this test resulted in an OOM - so the backing store references we hold are strong. Turning the GC preamble back on, they were then cleared again.
That is, the memory model here is sound in both directions - the kj::Array strong references are sound, and the memory collection model is sound.
This is great work, and ready to ship, thank you @logan-gatlin!
Remove the terminatedByteOffset fast-path from shouldRetain() since GC-based cleanup via the weak instanceRef is sufficient for all entry types. Remove three redundant per-permutation shouldRetain/filter tests that now tested identical logic; the mixed list test covers shouldRetain across all permutations.
ceb8ee6 to
501f6ec
Compare
npaun
left a comment
There was a problem hiding this comment.
LGTM; Guy's changes are a straightforward removal of an unneeded fast path.
Currently,
__instance_signalis ignored if__instance_terminatedis not present. This is because registering a signal creates a strong reference to the instance's memory, and we need some indication of when it is safe to free it. Keeping a weak reference to the memory is not an option, since weak references are not safe to use in a signal handler.The new solution is to keep a weak reference to the instance alongside our strong reference to its memory. Then in the GC prologue, in addition to dropping references to memories that have
__instance_terminatedset, we will also drop memories when our instance weakref is now dead. WASM memories have independent lifetimes from the instance they were spawned from. This is because the instance that created a memory may die while another instance that still lives has imported its memory. Therefore, our strong reference to the memory will not prevent the instance from being GC'd.