Closed Bug 1031529 Opened 10 years ago Closed 10 years ago

Remove JS_THREADSAFE and add a --no-threads shell flag

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: gkw, Assigned: jandem)

References

Details

Attachments

(4 files)

After an IRC conversation, it was agreed upon that we should replace JS_THREADSAFE with a shell flag, probably --disable-threads. (not --disable-threadsafe)

One should note how this is related/not related to --ion-offthread-compile=[on|off] though. (The former is not ion-specific, the latter is ion-specific?)

Bug 720018 and bug 859545 may or may not be related.
Flags: needinfo?(jdemooij)
--disable-threads should imply --ion-offthread-compile=off, but not vice versa.
Steve can we land bug 975011? Gary suggested we fix that bug first.
Flags: needinfo?(jdemooij) → needinfo?(sphink)
(I'd be happy to take this bug btw once that's fixed.)
Bug 975011 finally stuck.
Flags: needinfo?(sphink)
(In reply to Steve Fink [:sfink], PTO Jul 28-Aug 6 from comment #4)
> Bug 975011 finally stuck.

Excellent, taking this.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
\o/
Summary: Removing JS_THREADSAFE, and add a --disable-threads shell flag → Remove JS_THREADSAFE and add a --no-threads shell flag
This patch adds a --no-threads shell flag to disable PJS threads and helper threads. The fuzzers can use this flag instead of --disable-threadsafe builds, to make problems easier to reproduce.

Note that the shell watchdog thread, for timeout(), is still allowed: --disable-threadsafe builds currently use SIGALRM, that's not more deterministic than the watchdog thread.
Attachment #8460826 - Flags: review?(bhackett1024)
This patch gets rid of the JS_THREADSAFE defines everywhere.

 45 files changed, 53 insertions(+), 777 deletions(-)
Attachment #8460827 - Flags: review?(bhackett1024)
Attachment #8460827 - Attachment description: Part 2 - rm JS_THREADSAFE defines → Part 2 - rm JS_THREADSAFE ifdefs
All builds will be threadsafe, so this patch stops defining JS_THREADSAFE (previous patch removed all uses) and removes the --disable-threadsafe code.
Attachment #8460828 - Flags: review?(mh+mozilla)
Attachment #8460831 - Flags: review?(bhackett1024)
Comment on attachment 8460826 [details] [diff] [review]
Part 1 - Add shell flag

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

::: js/src/vm/HelperThreads.cpp
@@ +422,5 @@
>  
>      if (threads)
>          return;
>  
> +    MOZ_ASSERT(CanUseExtraThreads());

maybe move this assert further up?
Attachment #8460826 - Flags: review?(bhackett1024) → review+
Comment on attachment 8460827 [details] [diff] [review]
Part 2 - rm JS_THREADSAFE ifdefs

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

Yay!  Can you remove JS_ION next?
Attachment #8460827 - Flags: review?(bhackett1024) → review+
Attachment #8460831 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett (:bhackett) from comment #12)
> Yay!  Can you remove JS_ION next?

Unfortunately people are using SM on platforms for which we don't have a JIT, and I think it'd be nice to support this. Maybe we could move more code outside JS_ION though...
Green on Try, FWIW:

https://tbpl.mozilla.org/?tree=Try&rev=8d97a21e435d

(There's some check_spidermonkey_style.py orange caused by a bad #include order somewhere; should be easy to fix.)
(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to Brian Hackett (:bhackett) from comment #12)
> > Yay!  Can you remove JS_ION next?
> 
> Unfortunately people are using SM on platforms for which we don't have a
> JIT, and I think it'd be nice to support this. Maybe we could move more code
> outside JS_ION though...

That was a bit tongue in cheek but I think it would be really nice to do --- we would still be supporting --disable-ion builds, but just change out the files which are compiled rather than use an #ifdef everywhere.  Filed bug 1042833.
Attachment #8460828 - Flags: review?(mh+mozilla) → review+
Flags: qe-verify-
Blocks: 1253466
You need to log in before you can comment on or make changes to this bug.