Closed Bug 907777 Opened 6 years ago Closed 6 years ago

Add preference for off thread parsing

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bhackett, Unassigned)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Currently off thread JS parsing always happens if there are threads available for it.  As for off thread Ion compilation, this should be controlled using a pref.  The attached patch adds such an option, and changes the existing option for Ion compilation to behave similarly (it is currently a process wide boolean in js_IonOptions).  Bill, can you look at the new preference, and Jan, can you look at the changes to the existing Ion one?
Attachment #793523 - Flags: review?(wmccloskey)
Attachment #793523 - Flags: review?(jdemooij)
Comment on attachment 793523 [details] [diff] [review]
patch

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

::: dom/base/nsJSEnvironment.cpp
@@ +788,5 @@
>  
>    ::JS_SetOptions(context->mContext, newDefaultJSOptions & JSOPTION_MASK);
>  
> +  ::JS_SetParallelParsingEnabled(context->mContext, parallelParsing);
> +  ::JS_SetParallelIonCompilationEnabled(context->mContext, parallelIonCompilation);

I think we should also update dom/workers/RuntimeService to use these flags.

::: js/src/jit/shared/Assembler-shared.h
@@ +275,1 @@
>              JS_ASSERT_IF(MaybeGetIonContext() && !GetIonContext()->runtime->hadOutOfMemory, !used());

Does this work with parallel compilation enabled? I thought the MaybeGetIonContext() stuff was there because GetIonContext() is NULL (or triggers an assert) in that case.

::: js/src/jsapi.cpp
@@ +4838,5 @@
>  
>  JS_PUBLIC_API(bool)
>  JS::CanCompileOffThread(JSContext *cx, const CompileOptions &options)
>  {
>  #if defined(JS_THREADSAFE) && defined(JS_ION)

It's pre-existing, but why the defined(JS_ION) check? Because we only create worker threads in that case? Would be good to fix that separately if possible, when I saw it I thought this was about Ion-compilation instead of bytecode compilation.

::: js/src/shell/js.cpp
@@ +5162,1 @@
>          } else if (strcmp(str, "off") == 0) {

This should be != I think. If it's not "off" or "on", it's an error.
(In reply to Jan de Mooij [:jandem] (PTO Aug 22 and Aug 23) from comment #1)
> Comment on attachment 793523 [details] [diff] [review]
> patch
> 
> Review of attachment 793523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsJSEnvironment.cpp
> @@ +788,5 @@
> >  
> >    ::JS_SetOptions(context->mContext, newDefaultJSOptions & JSOPTION_MASK);
> >  
> > +  ::JS_SetParallelParsingEnabled(context->mContext, parallelParsing);
> > +  ::JS_SetParallelIonCompilationEnabled(context->mContext, parallelIonCompilation);
> 
> I think we should also update dom/workers/RuntimeService to use these flags.

DOM workers don't use helper threads, so even if these are enabled we won't parse or compile anything off thread.

> ::: js/src/jit/shared/Assembler-shared.h
> @@ +275,1 @@
> >              JS_ASSERT_IF(MaybeGetIonContext() && !GetIonContext()->runtime->hadOutOfMemory, !used());
> 
> Does this work with parallel compilation enabled? I thought the
> MaybeGetIonContext() stuff was there because GetIonContext() is NULL (or
> triggers an assert) in that case.

Probably not, I'll fix this.  Would be nice if it there was a comment on the MaybeGetIonContext call, or if it wasn't necessary in the first place (looks like some OdinMonkey thing).

> ::: js/src/jsapi.cpp
> @@ +4838,5 @@
> >  
> >  JS_PUBLIC_API(bool)
> >  JS::CanCompileOffThread(JSContext *cx, const CompileOptions &options)
> >  {
> >  #if defined(JS_THREADSAFE) && defined(JS_ION)
> 
> It's pre-existing, but why the defined(JS_ION) check? Because we only create
> worker threads in that case? Would be good to fix that separately if
> possible, when I saw it I thought this was about Ion-compilation instead of
> bytecode compilation.

This should be JS_WORKER_THREADS, I'll change it (just an alias for JS_THREADSAFE && JS_ION).  Originally we just used the worker threads for Ion compilation so both of these were necessary to do anything, now with off thread parsing that could be changed I guess but I'd like to leave it alone to avoid a profusion of #ifdefs.

> ::: js/src/shell/js.cpp
> @@ +5162,1 @@
> >          } else if (strcmp(str, "off") == 0) {
> 
> This should be != I think. If it's not "off" or "on", it's an error.

Fixed.
Attachment #793523 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/caecd32b3f33
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Attachment #793523 - Flags: review?(jdemooij) → review+
You need to log in before you can comment on or make changes to this bug.