Make __instance_signal and __instance_terminated fully indepedent #6270
Open
logan-gatlin wants to merge 4 commits intomainfrom
Open
Make __instance_signal and __instance_terminated fully indepedent #6270logan-gatlin wants to merge 4 commits intomainfrom
logan-gatlin wants to merge 4 commits intomainfrom
Conversation
Contributor
|
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
|
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.
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.