Closed Bug 1278635 Opened 4 years ago Closed 4 years ago

Implement about:config pref for Wasm baseline JIT

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(1 file, 1 obsolete file)

See bug 1232205.  Eventually the baseline JIT will be enabled by default, but for now we need to be able to enable it explicitly, and eventually we'll want to be able to disable it.

(Memo to self: see bug 1231337 for a similar case)
Attached patch bug1278635-baseline-pref.patch (obsolete) — Splinter Review
Work in progress, for safekeeping.
For the shell we currently have --wasm-always-baseline, meaning force the baseline JIT.

For the browser it seemed more appropriate to go with j.o.wasm_baselinejit, defaulting to false for now and defaulting to true later, meaning the baseline JIT is disabled/enabled, not implying anything about whether it is forced or not.

At the moment "true" for this pref forces the baseline JIT, but it seems likely we would change that behind the scenes eventually.
Attachment #8760859 - Attachment is obsolete: true
Attachment #8761146 - Flags: review?(luke)
Comment on attachment 8761146 [details] [diff] [review]
bug1278635-baseline-pref.patch

Review of attachment 8761146 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1637,5 @@
>      JS::RuntimeOptionsRef(rt).setBaseline(useBaseline)
>                               .setIon(useIon)
>                               .setAsmJS(useAsmJS)
>                               .setWasm(useWasm)
> +                             .setWasmAlwaysBaseline(useWasmBaseline)

So in the future when we wanted to enable by default, we'd add a new setWasmBaselineEnabled() and set that instead here?  That seems reasonable.

::: modules/libpref/init/all.js
@@ +1186,5 @@
>  pref("javascript.options.baselinejit",      true);
>  pref("javascript.options.ion",              true);
>  pref("javascript.options.asmjs",            true);
>  pref("javascript.options.wasm",             false);
> +pref("javascript.options.wasm_baselinejit", false);

I noticed we already have "javascript.options.ion" and "javascript.ion.offthread_compilation" which suggests "javascript.options.wasm.baselinejit".  At some point in time we should consider starting a new top-level "wasm"...
Attachment #8761146 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #3)
> Comment on attachment 8761146 [details] [diff] [review]
> bug1278635-baseline-pref.patch
> 
> Review of attachment 8761146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/init/all.js
> @@ +1186,5 @@
> >  pref("javascript.options.baselinejit",      true);
> >  pref("javascript.options.ion",              true);
> >  pref("javascript.options.asmjs",            true);
> >  pref("javascript.options.wasm",             false);
> > +pref("javascript.options.wasm_baselinejit", false);
> 
> I noticed we already have "javascript.options.ion" and
> "javascript.ion.offthread_compilation" which suggests
> "javascript.options.wasm.baselinejit".  At some point in time we should
> consider starting a new top-level "wasm"...

Should we figure this out now?  If not I'm inclined to land as-is and tidy up later once we've had a discussion about it.  (The window for discussion is a couple of days, while Benjamin is reviewing the big patch.)
https://hg.mozilla.org/mozilla-central/rev/f7e83f55d492
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.