Chore: Update V8 flag initialization to use V8 API #458
Conversation
Signed-off-by: Rachel Green <[email protected]>
rachgreen33
changed the title
| static wasm::own<wasm::Engine> engine; | ||
|
|
||
| std::call_once(init, []() { | ||
| ::v8::internal::v8_flags.liftoff = false; |
There was a problem hiding this comment.
But why? It was written years ago, but at the time loading Wasm module wouldn't complete before all compilation phases completed, so Liftoff compilation was wasteful. Is that not the case anymore?
There was a problem hiding this comment.
Thanks for the callout on this. I discussed with Matt and we do want to keep this disabled - Updated.
There was a problem hiding this comment.
Keeping it SGTM, but I think it would be helpful to add a comment that provides some context on why we set it. Internal bug b/278233034 contains some pointers to past discussions. I'd suggest something like: "Disable Liftoff compiler to force optimized JIT up-front."
There was a problem hiding this comment.
Again, this is based on tests from years ago, but TurboFan was always blocking loading of Wasm modules, and this flag doesn't affect that. What it does is that it skips Liftoff compilation that would be discarded in favor of TurboFan anyway... It would be good to verify that's still the case, though.
There are some docs for V8 here: https://v8.dev/docs/wasm-compilation-pipeline
There was a problem hiding this comment.
We want Turbofan to block on loading of Wasm modules, right? Thanks for the docs link, it aligns with discussion above: --no-liftoff sets the "TurboFan only" mode. If liftoff is enabled, calls can immediately use the compiled code, and then re-compilation with TurboFan is triggered in the background only after a certain threshold of calls.
Signed-off-by: Rachel Green <[email protected]>
Signed-off-by: Rachel Green <[email protected]>
rachgreen33
changed the title
Signed-off-by: Rachel Green <[email protected]>
rachgreen33
requested review from
martijneken,
mpwarres and
leonm1
as code owners
| static wasm::own<wasm::Engine> engine; | ||
|
|
||
| std::call_once(init, []() { | ||
| ::v8::internal::v8_flags.liftoff = false; |
There was a problem hiding this comment.
Keeping it SGTM, but I think it would be helpful to add a comment that provides some context on why we set it. Internal bug b/278233034 contains some pointers to past discussions. I'd suggest something like: "Disable Liftoff compiler to force optimized JIT up-front."
Signed-off-by: Rachel Green <[email protected]>
This change aligns with the V8 team's recommendation of setting flags this way through their API, rather than directly referencing v8::internal::v8_flags.