Make --enable-threadsafe the default configuration

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

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

Comment 1

6 years ago
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
Assignee: general → terrence
Status: NEW → ASSIGNED
Attachment #814974 - Flags: review?(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.
Attachment #814974 - Flags: review?(jorendorff)
Attachment #814974 - Flags: review?(gps)
Attachment #814974 - Flags: review+
Assignee

Comment 3

6 years ago
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.
Attachment #814974 - Flags: review+ → superreview+
Attachment #814974 - Flags: review?(gps) → review+
Assignee

Comment 4

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

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

Updated

6 years ago
Depends on: 884617
In the spirit of testing the same configuration as the browser, --ion-parallel-compile=off by default (which all the concurrent Ion/Odin compilation).
Assignee

Updated

6 years ago
Blocks: 927685

Comment 7

6 years ago
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.
Flags: needinfo?(gary)
Flags: needinfo?(choller)
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.
Flags: needinfo?(gary) → needinfo?(terrence)
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.
Flags: needinfo?(choller)
https://hg.mozilla.org/mozilla-central/rev/07606a1ebf5d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(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.
> 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.
Assignee

Updated

6 years ago
Depends on: 927915
Assignee

Comment 13

6 years ago
(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.
Flags: needinfo?(terrence)
You need to log in before you can comment on or make changes to this bug.