Skip to content

Make __instance_signal and __instance_terminated fully indepedent #6270

Draft
logan-gatlin wants to merge 3 commits intomainfrom
logan/instance-lifetime-tracking
Draft

Make __instance_signal and __instance_terminated fully indepedent #6270
logan-gatlin wants to merge 3 commits intomainfrom
logan/instance-lifetime-tracking

Conversation

@logan-gatlin
Copy link
Contributor

Currently, __instance_signal is ignored if __instance_terminated is 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 freeing memories that have __instance_terminated set, we will also free memories where our instance weakref is now dead. I have confirmed through tests and by looking at V8 src code that a WASM memory does not keep the instance it is attached to alive

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 6, 2026

The old code had kj::mv(signalOffset) which was unnecessary for a kj::Maybe<uint32_t> (trivially copyable). The new code just copies both, which is fine.

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

github run

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 6, 2026

Merging this PR will degrade performance by 19.65%

❌ 2 regressed benchmarks
✅ 68 untouched benchmarks
⏩ 129 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
Encode_ASCII_1024[TextEncoder][0/0/1024] 3.6 ms 4 ms -9.8%
bodyWithHeaders[Response] 30.4 µs 37.8 µs -19.65%

Comparing logan/instance-lifetime-tracking (eb84a1c) with main (fd0665d)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant