Brendan/shaver indicate that JS_THREADSAFE is going away before FF4. I'd like to remove the --enable-threadsafe option and turn JS_THREADSAFE on by default. This will result in build errors in the shell if NSPR isn't somehow available. I suppose the best option is to fail at configure time if neither of --with-system-nspr --with-nspr-libs is provided.

This will be like the last thing to do, or nearly so. Other bugs to fix first include bug 566951 and (before it) bug 558451. /be

Yes, but I have an ultierior motive. Performance is different whether using threadsafe or not. I spent a few hours today and yesterday chasing down two performance regressions which went away with --enable-threadsafe. Also, I think that, like in bug 580409, measuring without JS_THREADSAFE on confuses us. We're measuring the wrong thing. So it should be on by default.

Fair point -- we've been in la-la land forever with the non-threadsafe shell. /be

(In reply to comment #0) > Brendan/shaver indicate that JS_THREADSAFE is going away before FF4. I'd like > to remove the --enable-threadsafe option and turn JS_THREADSAFE on by default. > > This will result in build errors in the shell if NSPR isn't somehow available. > I suppose the best option is to fail at configure time if neither of > > --with-system-nspr > --with-nspr-libs > > is provided. Please try to make sure it's still about as easy and reliable to do a build as it is now. Now is not the time to be chasing down obscure build system issues.

This adds a warning if none of --with-nspr-libs --with-nspr-cflags and --with-system-nspr are set. The --enable-threadsafe option is removed, and JS_THREADSAFE is always set. configure does _not_ fail on --enable-threadsafe, which I guess is fine.

(In reply to comment #4) > Please try to make sure it's still about as easy and reliable to do a build as > it is now. Now is not the time to be chasing down obscure build system issues. The patch from comment 5 alerts you if you're missing flags for NSPR. This is a step up over the first time I tried --enable-threadsafe, when configure worked fine, and I got a compile error instead.

Comment on attachment 467063 [details] [diff] [review] Configury fix I'm not that guy! /be

This is a slightly different approach than the last patch. In the last patch, people were required to pass --with-system-nspr or a combination of --with-nspr-cflags/libs. In this patch, using the system NSPR is the default, so we no longer need --with-system-nspr. We turn off the system NSPR if the other flags are provided, which removes the need to have an error message. I tested this by configuring with each of: nothing (uses the system NSPR) --with-nspr-cflags --with-nspr-libs --with-system-nspr (changes nothing) -- enable-threadsafe (changes nothing) I'd like to get this in soon, as I spent all yesterday on a bug which only occurred in non-NSPR builds. I note that lots of (possibly most) people use non-NSPR builds by default (for example: the fuzzers, AWFY, many bug reports), and that by turning it on by default we'll stop reporting bugs that can't happen in Firefox, as well as getting more accurate performance results.

Comment on attachment 500273 [details] [diff] [review] Remove --enable-threadsafe and --with-system-nspr This comment needs to be fixed: dnl Top-level Mozilla switched to requiring NSPR 4.8.6 (bug 560582), but we don't need it in JS. Looks fine otherwise.

This should also remove the JS_THREADSAFE #define then.

(In reply to comment #10) > This should also remove the JS_THREADSAFE #define then. I deliberately didn't do this here for a few reasons: - FF4 is coming, and that's a big change that might not get approved - FF4 is coming, and we should be doing more important things - I wanted to make this easy to approve - The patch to remove all the JS_THREADSAFE stuff will bitrot very easily. I'll create a new bug to track removing JS_THREADSAFE, but I'd suggest that it be removed incrementally until I'm bored with a blocker and take the time to rip out what's left.

(In reply to comment #9) > This comment needs to be fixed: > > dnl Top-level Mozilla switched to requiring NSPR 4.8.6 (bug 560582), but we > don't need it in JS. I'm not sure what you mean for me to fix. This is still true I think.

(In reply to comment #12) > > This comment needs to be fixed: > > > > dnl Top-level Mozilla switched to requiring NSPR 4.8.6 (bug 560582), but we > > don't need it in JS. > > > I'm not sure what you mean for me to fix. This is still true I think. We resolved this on IRC. The comment was ambiguous, and jimb thought it meant NSPR wasn't required. I've fixed it to be unambiguous.

Backed out: //hg.mozilla.org/tracemonkey/rev/088038e20a55 Although this works fine, dmandelin was unable to build windows with NSPR, so we can't really force this on until that's resolved. We need: 1) to figure out how the build should work 2) to document it on https://developer.mozilla.org/En/SpiderMonkey/Build_Documentation

See also bug 625895

Enabling/disabling threadsafe mode became a runtime flag (--no-threads, for example), and it is now possible to compile the js shell on Linux with NSPR using just --enable-nspr-build, so marking WFM.