924986 - Make --enable-threadsafe the default configuration

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

It is extremely error prone not to have this as the default build now that we depend heavily on threads.

Comment 1

[Terrence Cole [:terrence]]

Attachment: threadsafe_by_default-v0.diff

Tested locally. Try should already be --enable-threadsafe everywhere (except for where it isn't), but let's throw up a try run just to be on the safe side:

https://tbpl.mozilla.org/?tree=Try&rev=2a4259db61ce

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 814974 [details] [diff] [review]
threadsafe_by_default-v0.diff

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

<jorendorff>	 the only argument i can think of against is that it's easier to build --disable-threadsafe
<jorendorff>	 which isn't much of an argument
<till>	 jorendorff: I strongly support this. I saw quite a few new people that didn't even know about threadsafe builds and were chasing bogus perf issues or having a hard time getting their patches right because of it
<terrence>	 jorendorff: it bit us because we forgot to include --enable-threadsafe in the SM(ggc) builds which we're trying to show on tbpl on inbound
<terrence>	 jorendorff: so some threading related regressors landing, which we're having to clean up now

r=me.

I think remove --enable-threadsafe from /configure.in as well.

I can't really claim competence to review stuff in the build system, so requesting addl. r?gps.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 814974 [details] [diff] [review]
threadsafe_by_default-v0.diff

I meant to sr? that. Upgrading Jason's r+ to sr+, after asking on IRC.

Comment 4

[Terrence Cole [:terrence]]

And retrying on current tip, since windows was orange and red on my tip yesterday.

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

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07606a1ebf5d

Comment 6

[Luke Wagner [:luke]]

In the spirit of testing the same configuration as the browser, --ion-parallel-compile=off by default (which all the concurrent Ion/Odin compilation).

Comment 7

[Shu-yu Guo [:shu]]

Nice! Will this affect fuzzing as well so that I can assume all fuzz bugs in the future are from threadsafe builds? Requesting needinfo from Gary and Christian.

Comment 8

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

Please don't remove --enable-threadsafe, make it a no-op or something (similar to --no-jm?) - we don't want to break compatibility with autoBisect and the harness running older builds.

And yes, this will affect fuzzing except that we'll be fuzzing threadsafe builds by default now, so that's the default configuration that people should test on going forward.

Comment 9

[Christian Holler (:decoder)]

How does this behave in terms of nspr linkage? Does it automatically use nspr from the tree and does it statically link to that, so the shell can just be used? So far, I've been struggling with threadsafe builds because I had to install a recent (self-compiled) version of nspr on the system, with both 32 and 64 bit.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07606a1ebf5d

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Christian Holler (:decoder) from comment #9)
> How does this behave in terms of nspr linkage? Does it automatically use
> nspr from the tree and does it statically link to that, so the shell can
> just be used? So far, I've been struggling with threadsafe builds because I
> had to install a recent (self-compiled) version of nspr on the system, with
> both 32 and 64 bit.

I think it doesn't automatically use anything; plain './configure && make' no longer works. You *must* specify which nspr to use, either --with-system-nspr or (in your case) --with-nspr-prefix.

This is not great. Clearly we should have consulted you first. Maybe by default, we should build an NSPR for you, from the source in $SRCDIR/../../nsprpub, in a subdirectory of the JS objdir. (Is that roughly how we deal with ICU?)

Terrence, can you take a followup bug to deal with this? More trouble than the original bug, I'm afraid.

Comment 12

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

> Maybe by
> default, we should build an NSPR for you, from the source in
> $SRCDIR/../../nsprpub, in a subdirectory of the JS objdir. (Is that roughly
> how we deal with ICU?)

A while ago, I coded this manually into the jsfunfuzz harness to allow it to compile threadsafe builds (then passed in the path for --with-nspr-prefix), which is similar to this idea. It has worked well so far.

Comment 13

[Terrence Cole [:terrence]]

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> (In reply to Christian Holler (:decoder) from comment #9)
> > How does this behave in terms of nspr linkage? Does it automatically use
> > nspr from the tree and does it statically link to that, so the shell can
> > just be used? So far, I've been struggling with threadsafe builds because I
> > had to install a recent (self-compiled) version of nspr on the system, with
> > both 32 and 64 bit.

Sorry about that! I assumed that your default fuzzing set already included --enable-threadsafe.

> I think it doesn't automatically use anything; plain './configure && make'
> no longer works. You *must* specify which nspr to use, either
> --with-system-nspr or (in your case) --with-nspr-prefix.

Or --disable-threadsafe :-/.

> This is not great. Clearly we should have consulted you first. Maybe by
> default, we should build an NSPR for you, from the source in
> $SRCDIR/../../nsprpub, in a subdirectory of the JS objdir. (Is that roughly
> how we deal with ICU?)

This is a great idea.

> Terrence, can you take a followup bug to deal with this? More trouble than
> the original bug, I'm afraid.

Filed bug 927915. I'm no expert at build systems in general and our build system in particular, but ICU should be a solid starting point.