Closed Bug 1284808 Opened 3 years ago Closed 3 years ago

Move RuntimeOptions to JSContext

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
This renames the RuntimeOptions class to ContextOptions and moves it to the context.

It revealed a few places where background threads used these options (that's racy). I fixed that by moving the options to ExclusiveContext, so each background thread now gets a (read-only) copy of the main thread's ContextOptions.

Luke, asking you to review this so you're aware of these changes (it conflicts with the no-cx wasm patches).

r? baku for the dom/ changes.
Attachment #8768348 - Flags: review?(luke)
Attachment #8768348 - Flags: review?(amarchesini)
Comment on attachment 8768348 [details] [diff] [review]
Patch

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

::: dom/workers/WorkerPrivate.cpp
@@ +1451,2 @@
>                                      WorkerPrivate* aWorkerPrivate,
> +                                    const JS::ContextOptions& aContextOptions)

can you fix the indentation here?

Something like:

UpdateContextOptionsRunnable(WorkerPrivate* aWorkerPrivate,
                             const JS::ContextOptions& aContextOptions)
Attachment #8768348 - Flags: review?(amarchesini) → review+
Comment on attachment 8768348 [details] [diff] [review]
Patch

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

::: js/src/asmjs/WasmGenerator.cpp
@@ +820,5 @@
>                                            Move(fg->callSiteLineNums_));
>      if (!func)
>          return false;
>  
> +    bool baselineCompile = cx_->options().wasmAlwaysBaseline() && BaselineCanCompile(fg);

Ah, I see I am one of the guilty ones :|  With bug 1284056 landed, this line lives in WasmCompile.cpp : CompileArgs::init(JSContext*).

::: js/src/jscntxt.h
@@ +119,5 @@
>  
> +  protected:
> +    // Background threads get a read-only copy of the main thread's
> +    // ContextOptions.
> +    JS::ContextOptions options_;

Hah, yes, much less racy.
Attachment #8768348 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc866385dd01
Rename RuntimeOptions to ContextOptions and move it to the context. r=luke,baku
https://hg.mozilla.org/mozilla-central/rev/cc866385dd01
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
This change appears to be causing serious problems for some Linux users. See bug 1285243.
(In reply to B.J. Herbison from comment #5)
> This change appears to be causing serious problems for some Linux users. See
> bug 1285243.

According to bug 1285243 comment 6, it's not this bug, but bug 1284440.
You need to log in before you can comment on or make changes to this bug.