Closed Bug 1697560 Opened 3 years ago Closed 3 years ago

wasm JIT preferences are sometimes ignored

Categories

(Core :: JavaScript: WebAssembly, defect, P2)

Product:
Core
Component:
JavaScript: WebAssembly
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
90 Branch
Tracking Flags:
Tracking Status
firefox90 --- fixed

People

(Reporter: jseward, Assigned: lth)

References

Details

Attachments

(2 files)

15-verbose-compilation-start.diff
5.63 KB, patch
Details | Diff | Splinter Review
Bug 1697560 - Compute asm.js pref correctly for CompileOptions. r?jseward
48 bytes, text/x-phabricator-request
Details | Review

When running wasm in a full browser build, it can happen that some about:config
options are ignored. I believe this happens, but is impossible to observe, in
unmodified m-c sources. However, when the attached patch is applied, the
problem is obvious.

STR: build on x86_64-linux. Start. Ensure that

  javascript.options.wasm_baselinejit = true
  javascript.options.wasm_optimizingjit = false

(meaning that we request Baseline-only). Load

https://beta.unity3d.com/jonas/AngryBots

and observe that only Ion compiles are initiated -- 40366 of them.

By contrast, loading

https://www.funkykarts.rocks/demo.html

produces only Baseline compilation, as expected.

I hypothesise (guess wildly, without much insight) that this is somehow to do
with the fact that AngryBots test case gets a child process in order to do
WebGL stuff (or something like that), whilst Funky Karts doesn't.

Patch that prints a line for each wasm function compiled, so as to make it
easy to figure out when compilation is happening and which compiler has
been used.

Assignee: nobody → jseward

The first thing to do might be to carefully read docs for StaticPrefs.yml to see if we're doing something stupid / something that should be done differently. Or experiment with moving those prefs into the regular prefs to see what happens.

For the JIT prefs we've moved to process-wide flags that are only read on startup (see LoadStartupJSPrefs). That way we don't have to worry about setting the right JSContext options for DOM workers, worklets, the PAC thread etc.

Blocks: 1678097
Severity: -- → S3
Status: NEW → ASSIGNED
Priority: -- → P1

Possibly relevant (although not the cause of this bug):
bug 1697904 (Migrate more JS prefs to StaticPrefs)

Results of investigation so far (with much help from Jan):

For wasm compilation initiated by DOM worker threads, default options are
created in ContextOptions::ContextOptions(). That is copied into a JSContext
by JSContext::JSContext(). The copy is later updated by a call to
InitJSContextForWorker(), and so acquires the correct values.

For wasm compilation initiated by the main thread, default options are updated
by a call to ReloadPrefsCallback().

The problem occurs for asm.js compilations triggered by a parse task, running
in a JS helper thread. There [1], IsAsmJSCompilerAvailable() is called using
a (shared) JSContext which possibly does not have an up-to-date options_
field.

Currently I have no plan for how to fix it.

[1] https://searchfox.org/mozilla-central/rev/9ae77e4ce3378bd683ac9a86b729ea6b6bd22cb8/js/src/wasm/AsmJS.cpp#7031

See Also: → 1698473

I think this is a serious problem, because it affects other JS prefs too, see bug 1698473. ReloadPrefsCallback is not called for the worker's JSContext, so it only gets default settings. This could be an old bug.

I should add (which I forgot, earlier) .. this isn't related to the process structure.
At least for this case. For Lars' possibly-related bug 1698473, I can't say.

For this bug, I can reproduce it (show that wrong prefs are getting used, via
debug printing) purely in the firefox "root" process. Although my minimal test
case involves both a root process and one content process, debug printing
showed the problem exists within the root process. So it must be a thread-
structure thing, per comment 5.

The wasm_baselinejit pref was not being mirrored to Workers, but I've fixed that on https://phabricator.services.mozilla.com/D108491 .

(In reply to Ted Campbell [:tcampbell] from comment #8)

The wasm_baselinejit pref was not being mirrored to Workers, [..]

So .. (unclear) .. are you saying that that will fix this problem?
I kinda suspect not, because this was about the wasm_optimizingjit
pref not being mirrored to JS helper threads.

The wasm_baselinejit pref seemed wrong for workers, but based on your observations might also be wrong for other reasons, so probably not solved.

Looking at this again, I now understand what you are saying. Yes, I agree that HelperThreads don't have up-to-date ContextOptions. For JS-parsing, I tried to ensure that CompileOptions snapshotted what it needed from the JSContext while on main-thread. For other types of options, we use JitOptions as a global option state. These few WASM options seem to fit in their own category of ContextOptions.

Migrating them to JitOptions probably makes the most sense if they can be global state. In the future I'd like to auto-generate most of the JitOptions state from the StaticPrefsList.yaml file.

Priority: P1 → P2

Ship blocker for ARM64 Ion (FF90), though as we're disabling Cranelift at the time we can discuss to what extent it needs to be fixed by that milestone.

Blocks: 1687626

In addition to the above, we probably have no clear policy about whether asm.js compilation should pay attention to the wasm compiler selection, even if the asm.js compiler generates wasm that we then compile using the wasm compiler. I think this could be argued either way; and if we cared about asm.js much, we might worry about it further.

To summarize:

  • this appears to affect only asm.js, for a well understood reason (comment 5, comment 11)
  • with cranelift out of the picture, asm.js compilation gets only ion, which is available everywhere

A point fix that might address the issue is to update JS::CompileOptions::CompileOptions (link in comment 11) so that asm.js is disabled also if the wasm optimizing jit is unavailable or if cranelift is selected. That way, the asm.js setting is the one that controls everything. Calling IsAsmJSCompilerAvailable(cx) is possibly the right remedy.

I'll take this to play around with that, but at this stage this bug probably doesn't block shipping Ion on ARM64 in any serious way.

Assignee: jseward → lhansen
Blocks: 1709803

The computation of CompileOptions captures only the asmjs flag value,
but this is insufficient to determine if asmjs compilation is actually
supported: we must also determine whether a wasm compiler is available
to compile the wasm that is emitted by the asmjs front end.

This has been a pretty typical error pattern for us in the past, hence
the introduction of the class of wasm::WhateverAvailable(cx)
predicates to work around this. That fix needs to be used here too.

This patch adds the necessary check to the computation of
CompileOptions and for maximum clarity also distinguishes this case
from the case where the asmjs pref is turned off, even though that's
not necessary. A couple of new predicates are introduced to keep the
check clean.

Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ebf02188fbf
Compute asm.js pref correctly for CompileOptions. r=jseward

Test case enumerates all possible (and some impossible) error messages; we just need to add one more, because the patch adds one more.

Flags: needinfo?(lhansen)
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3a25413321c
Compute asm.js pref correctly for CompileOptions. r=jseward

https://hg.mozilla.org/mozilla-central/rev/b3a25413321c

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox90: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: