[wasm] Ensure NativeModule alive throughout bg compilation [v8/v8 : main]
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +1 |
1 comment
void KeepAliveWasmNativeModule(Not sure if there's a better solution.
Storing the `shared_ptr` in `JSWasmCallParameters` doesn't seem to be possible because of it being kept in a zone without destructors being called.
Related details
- Clemens Backes
- Daniel Lehmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
This is not my area of expertise, so no idea if there's a better way. This implementation looks overly specific.
Can't we keep the NativeModule alive via the `WasmTrustedInstanceData`, which links to the `TrustedManaged`? Are those objects maybe already held alive via the broker?
Related details
- Daniel Lehmann
- Maksim Ivanov
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 2 comments![]( "Open in Gerrit")
This is not my area of expertise, so no idea if there's a better way. This implementation looks overly specific.
Can't we keep the NativeModule alive via the `WasmTrustedInstanceData`, which links to the `TrustedManaged`? Are those objects maybe already held alive via the broker?
As for your last question
> Are those objects maybe already held alive via the broker?
Apparently not, otherwise we couldn't trigger this via the reproducer on the bug, no?
Generally, I agree though: Can't we keep the `WasmTrustedInstanceData` alive during compilation somehow? Isn't that what the `JSHeapBroker`, `PersistentHandles`, and these `*Ref` classes are for in the first place?
void KeepAliveWasmNativeModule(Not sure if there's a better solution.
Storing the `shared_ptr` in `JSWasmCallParameters` doesn't seem to be possible because of it being kept in a zone without destructors being called.
I am unfortunately not an expert either in how to best keep Wasm C++ objects alive during background compilation. However, I have a gut feeling that this particular implementation is (1) overly specific to this concrete issue, and (2) feels a bit hacky/bolted on. I tried to brainstorm a bit with Gemini (and given my non-expertise, I hope this isn't totally off): Can we
1. Implement Broker Refs: Define `WasmExportedFunctionDataRef` and `WasmTrustedInstanceDataRef` in `src/compiler/js-heap-broker.h`.
2. Serialize on Main Thread: In `JSCallReducer` (on the main thread), instead of reading raw `Tagged` pointers, serialize the target function's trusted data into these `*Ref` classes.
3. Keep compiler raw pointers: The compiler can then safely continue using raw `wasm::NativeModule*` pointers in the Zone, because the broker's persistent handles guarantee their lifetime.
Given that Clemens and me are not the experts in this, who would be? Jakob Linke and Leszek, since they worked on the heap broker more than us, maybe they can briefly give feedback on this idea?
Related details
- Maksim Ivanov
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 1 comment![]( "Open in Gerrit")
Daniel LehmannThis is not my area of expertise, so no idea if there's a better way. This implementation looks overly specific.
Can't we keep the NativeModule alive via the `WasmTrustedInstanceData`, which links to the `TrustedManaged`? Are those objects maybe already held alive via the broker?
As for your last question
> Are those objects maybe already held alive via the broker?Apparently not, otherwise we couldn't trigger this via the reproducer on the bug, no?
Generally, I agree though: Can't we keep the `WasmTrustedInstanceData` alive during compilation somehow? Isn't that what the `JSHeapBroker`, `PersistentHandles`, and these `*Ref` classes are for in the first place?
Thanks for the idea! I tried implementing this and saw it can be done via `CanonicalPersistentHandle`. Not sure if it's correct and not too "magic" - PTAL.
Related details
- Clemens Backes
- Daniel Lehmann
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
1 comment
Related details
- Clemens Backes
- Daniel Lehmann
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Lehmann
| Code-Review | +1 |
Daniel LehmannThis is not my area of expertise, so no idea if there's a better way. This implementation looks overly specific.
Can't we keep the NativeModule alive via the `WasmTrustedInstanceData`, which links to the `TrustedManaged`? Are those objects maybe already held alive via the broker?
Maksim IvanovAs for your last question
> Are those objects maybe already held alive via the broker?Apparently not, otherwise we couldn't trigger this via the reproducer on the bug, no?
Generally, I agree though: Can't we keep the `WasmTrustedInstanceData` alive during compilation somehow? Isn't that what the `JSHeapBroker`, `PersistentHandles`, and these `*Ref` classes are for in the first place?
Thanks for the idea! I tried implementing this and saw it can be done via `CanonicalPersistentHandle`. Not sure if it's correct and not too "magic" - PTAL.
Nice, that looks much more elegant and idiomatic :)
LGTM
LGTM as well (provided this fixes the reproducer, which I guess you already checked).
Daniel LehmannNot sure if there's a better solution.
Storing the `shared_ptr` in `JSWasmCallParameters` doesn't seem to be possible because of it being kept in a zone without destructors being called.
I am unfortunately not an expert either in how to best keep Wasm C++ objects alive during background compilation. However, I have a gut feeling that this particular implementation is (1) overly specific to this concrete issue, and (2) feels a bit hacky/bolted on. I tried to brainstorm a bit with Gemini (and given my non-expertise, I hope this isn't totally off): Can we
1. Implement Broker Refs: Define `WasmExportedFunctionDataRef` and `WasmTrustedInstanceDataRef` in `src/compiler/js-heap-broker.h`.
2. Serialize on Main Thread: In `JSCallReducer` (on the main thread), instead of reading raw `Tagged` pointers, serialize the target function's trusted data into these `*Ref` classes.
3. Keep compiler raw pointers: The compiler can then safely continue using raw `wasm::NativeModule*` pointers in the Zone, because the broker's persistent handles guarantee their lifetime.
Given that Clemens and me are not the experts in this, who would be? Jakob Linke and Leszek, since they worked on the heap broker more than us, maybe they can briefly give feedback on this idea?
I think what Gemini proposed here goes a bit further still than your patchset now, but is not strictly necessary. More discussion for potential follow-ups on the (non-public) bug.
Related details
- Clemens Backes
- Maksim Ivanov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted Code-Review+1![]( "Open in Gerrit")
- Maksim Ivanov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +2 |
1 comment
void KeepAliveWasmNativeModule(Daniel LehmannNot sure if there's a better solution.
Storing the `shared_ptr` in `JSWasmCallParameters` doesn't seem to be possible because of it being kept in a zone without destructors being called.
Daniel LehmannI am unfortunately not an expert either in how to best keep Wasm C++ objects alive during background compilation. However, I have a gut feeling that this particular implementation is (1) overly specific to this concrete issue, and (2) feels a bit hacky/bolted on. I tried to brainstorm a bit with Gemini (and given my non-expertise, I hope this isn't totally off): Can we
1. Implement Broker Refs: Define `WasmExportedFunctionDataRef` and `WasmTrustedInstanceDataRef` in `src/compiler/js-heap-broker.h`.
2. Serialize on Main Thread: In `JSCallReducer` (on the main thread), instead of reading raw `Tagged` pointers, serialize the target function's trusted data into these `*Ref` classes.
3. Keep compiler raw pointers: The compiler can then safely continue using raw `wasm::NativeModule*` pointers in the Zone, because the broker's persistent handles guarantee their lifetime.
Given that Clemens and me are not the experts in this, who would be? Jakob Linke and Leszek, since they worked on the heap broker more than us, maybe they can briefly give feedback on this idea?
I think what Gemini proposed here goes a bit further still than your patchset now, but is not strictly necessary. More discussion for potential follow-ups on the (non-public) bug.
Ack - thanks for the additional brainstorm! Looks like there's still a lot we can do to improve clarity and robustness. Proceeding with the CL as an immediate fix.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[wasm] Ensure NativeModule alive throughout bg compilation
For the JS-to-Wasm inlining (JS call reducer and Turbolev graph
builder), make sure to keep the NativeModule alive throughout the
operation: even though it's owned through a TrustedManaged, the
ownership root isn't guaranteed to keep it alive.
NO_IFTTT=Inlining logic isn't changed
- M src/compiler/js-call-reducer.cc
- M src/compiler/turboshaft/turbolev-graph-builder.cc
Code-Review: +1 by Matthias Liedtke, +1 by Clemens Backes, +1 by Daniel Lehmann
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |