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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Shu-yu Guo [:shu]]

--disable-threads should imply --ion-offthread-compile=off, but not vice versa.

Comment 2

[Jan de Mooij [:jandem]]

Steve can we land bug 975011? Gary suggested we fix that bug first.

Comment 3

[Jan de Mooij [:jandem]]

(I'd be happy to take this bug btw once that's fixed.)

Comment 4

[Steve Fink [:sfink] [:s:]]

Bug 975011 finally stuck.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Steve Fink [:sfink], PTO Jul 28-Aug 6 from comment #4)
> Bug 975011 finally stuck.

Excellent, taking this.

Comment 6

[Shu-yu Guo [:shu]]

\o/

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add 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.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 2 - rm JS_THREADSAFE ifdefs

This patch gets rid of the JS_THREADSAFE defines everywhere.

 45 files changed, 53 insertions(+), 777 deletions(-)

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Build system changes

All builds will be threadsafe, so this patch stops defining JS_THREADSAFE (previous patch removed all uses) and removes the --disable-threadsafe code.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 4 - rm getBuildConfiguration().threadsafe

Comment 11

[Brian Hackett [Laid off!]]

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?

Comment 12

[Brian Hackett [Laid off!]]

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?

Comment 13

[Jan de Mooij [:jandem]]

(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...

Comment 14

[Jan de Mooij [:jandem]]

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.)

Comment 15

[Brian Hackett [Laid off!]]

(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.

Comment 16

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35038c3324ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0dd5a83ba36
https://hg.mozilla.org/integration/mozilla-inbound/rev/6426fef52f51
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8558ecd9b16

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/35038c3324ee
https://hg.mozilla.org/mozilla-central/rev/a0dd5a83ba36
https://hg.mozilla.org/mozilla-central/rev/6426fef52f51
https://hg.mozilla.org/mozilla-central/rev/e8558ecd9b16