841646 - Allow off-thread compilation when the SPS profiler is enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

+++ This bug was initially created as a clone of Bug #832812 +++

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

What's the status here? I know there's been a lot of work to make profiling work in different processes for B2G and the like, and bhackett's work in bug 785905 moved most of the remaining work in JS compilation off the main thread when off main thread compilation is enabled, making the potential perf difference here even bigger.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Bill, could you give an update? The workaround still seems to be in place:
https://hg.mozilla.org/mozilla-central/annotate/383ccc9eb488/js/src/ion/Ion.cpp#l1372

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It's still broken, but it's probably not too hard to fix. The strings are created here:
http://mxr.mozilla.org/mozilla-central/source/js/src/vm/SPSProfiler.cpp#77

One options would be to put a lock around the |strings| table. Another option would be to use separate tables for different threads. If we go with the lock approach, we'd probably want to measure how often the lock is acquired for a typical compilation (when SPS is enabled). Locks are pretty cheap, but I have no idea how many strings the profiler needs.

I don't have time to work on this.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Add lock around |strings| table to avoid races. v1

Thanks, that wasn't too hard. This patch adds a lock everywhere |strings| might get modified and removes the sps profiler checks from js/src/jit/Ion.cpp. Let me know if you're too busy to review this and I'll ask around :)

As for the patch itself:

Since the lookup itself is read-only, I decided to do one lookup outside the lock, and do a more expensive lookupForAdd inside the lock in case of a miss, on the theory that hits should be much more frequent than misses. I did some testing, and saw hits outweighing misses at least 10-to-1, so I think this holds up. Because the first lookup is read-only, I was able to replace it with readonlyThreadsafeLookup, which might even be slightly faster than before this patch (I think it's also needed, since regular lookup has a reentrancy assert).

I then tested the performance of several configurations: 1) no profiler, 2) with the new lock, but without parallel compilation enabled, 3) parallel compilation enabled with the new lock and 4) parallel compilation enabled but with no lock. The last configuration could cause crashes, but I wasn't able to reproduce any in my limited testing. At any rate, the results are as follows:

                  Octane   Sunspider
No profiler        26158     131.0ms
Lock               11936     229.7ms
Lock + Parallel    15033     196.0ms
Parallel           14967     196.4ms

So the patch helps ~26% on Octane and ~17% on Sunspider. Running without the profiler is still much faster; this is bug 958799. However it does show that adding the lock doesn't slow down running with the profiler on over the situation before bug 832812, so I'd say that's a win :)

One thing I'm not sure about: In my patch I also removed the check at https://hg.mozilla.org/mozilla-central/file/298f262f21ff/js/src/jit/Ion.cpp#l1761 , but this was added more recently. Since I haven't seen any crashes with these builds either way I can't tell if this is correct. In theory all the |strings| stuff should be thread-safe now, but I don't know if the off-thread MIR construction adds more constraints that affect the profiler.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8362003 [details] [diff] [review]
Add lock around |strings| table to avoid races. v1

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

Thanks very much Emanuel.

I'm afraid that readonlyThreadsafeLookup can't be used here. It only works when you know that none of the threads operating on the data structure can possibly perform writes. In your patch, another thread could be modifying the table while your thread is calling readonlyThreadsafeLookup. That could be bad if, for example, the writing thread was in the middle of resizing the hashtable.

Also, we usually use RAII when acquiring locks. Here's an example of such a class. You could just make one of your own.
http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Runtime.h#1904
It's better to it this way so that you don't forget to release the lock on a return path.

Otherwise, the patch looks good to me. However, please ask jandem for the next review. I'm not really familiar with the parallel compilation code.

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Bill McCloskey (:billm) from comment #5)
> I'm afraid that readonlyThreadsafeLookup can't be used here. It only works
> when you know that none of the threads operating on the data structure can
> possibly perform writes. In your patch, another thread could be modifying
> the table while your thread is calling readonlyThreadsafeLookup. That could
> be bad if, for example, the writing thread was in the middle of resizing the
> hashtable.

Ah, you're right.. that's unfortunate.

> Also, we usually use RAII when acquiring locks. Here's an example of such a
> class. You could just make one of your own.

Okay, looks easy enough.


> However, please ask jandem for the next review.

Will do :)

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Add lock around |strings| table to avoid races. v2

Alright, I added the SPSAutoLock class which locks upon construction and unlocks upon destruction, and got rid of the double lookup in SPSProfiler::profileString. The changes to SPSProfiler.cpp look a lot nicer now! And thankfully, performance seems unchanged on the major benchmarks.

jandem, this is my first time writing a class like this, so I might have missed some subtleties. I also don't really like the static member variable and having to initialize it outside the class, but I couldn't think of anything better. Other than that the patch is very straightforward now though.

Comment 8

[Benoit Girard (:BenWa)]

FWIW you may want to gather number with the profiler turned on but the sampling turn on to 1 second or above. Tweak profiler.interval to say 1000 and restart profiling. The overhead imposed by JS profiling is to track the pseudostack. The overhead to pause and sample the thread is independent of how much we tweak JS profiling and is very very platform specific. Just some food for though to better quantity the JS cost with less per platform noise.

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Add lock around |strings| table to avoid races. v3

Running v2 through try showed that I did overlook a few things in my auto-locking class. v3 should fix these by adding some extra checks; full try build coming up here (the ones I did before were more selective):
https://tbpl.mozilla.org/?tree=Try&rev=1872a705242c

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8362100 [details] [diff] [review]
Add lock around |strings| table to avoid races. v3

Ugh, still getting crashes. I'm cancelling review for now as I try to think of a solution, sorry about the churn.

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Add lock around |strings| table to avoid races. v4

Okay, I think this is ready for review now, try is looking good: https://tbpl.mozilla.org/?tree=Try&rev=285078b42427 I'm also a lot happier with the code :)

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8362149 [details] [diff] [review]
Add lock around |strings| table to avoid races. v4

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

Thanks for the patch! As discussed on IRC, the |GetIonContext()->cx| you see in CodeGenerator::visitFunctionBoundary will be nullptr if we're compiling off-thread (I wonder why that didn't crash with your patch, probably because all strings were already in the strings table so we didn't use it.)

We could refactor allocProfileString to not require a context, and then lock the strings table.

The altnernative, and maybe nicer/simpler is this: fix SPSInstrumentation::push and/or MacroAssembler::spsPushFrame to use movWithPatch(ImmWord(0), reg); This returns a CodeOffsetLabel (the location of the emitted "move" instruction). Then, in CodeGenerator, maintain a Vector of <JSScript, CodeOffsetLabel> structs. Then after compilation is done, we call CodeGenerator::link on the main thread and there we can loop over our vector and patch the CodeOffsetLabel pointers with the strings we get from SPS.

Let me know if you're interested in doing this and if you need help.

Comment 13

[Jan de Mooij [:jandem]]

Another problem: MIRGenerator::instrumentedProfiling() checks if SPS is enabled, this could race if the profiler is enabled/disabled on the main thread while we're compiling.

The simplest fix is to cancel all off-thread compilations whenever SPS is enabled/disabled (maybe we're doing that already, didn't check).

Comment 14

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 1: Refactor allocProfileString to not require a JSContext and remove JSContext * parameters from functions that no longer require them as a result

This patch removes the need for the JSContext parameter in allocProfileString by tallying up the length of each part beforehand, then allocating only once using the same contextless allocator that the function used before. It then uses JS_snprintf to put all the pieces together instead of doing manual appends.

One big note about this patch: allocProfileString also used the context to assert that no GCs happened during its execution - by removing the parameter I also had to remove this check. If GC can happen here, should we use something like AutoSuppressGC to suppress it? That would require a context, but IIRC GC is suppressed during off-thread compilation, so we could gate it on cx being non-null. (AutoSuppressGC does have a scary warning above it, but the comment in jscntxt.h it refers to seems to be missing ...)

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2: Add a lock around the |strings| table to avoid races and allow parallel compilation while profiling

I just rebased this on top of part 1 and made some tiny whitespace fixes. I'll wait until I've looked into comment #13 before asking for review again on this one.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8363779 [details] [diff] [review]
Part 1: Refactor allocProfileString to not require a JSContext and remove JSContext * parameters from functions that no longer require them as a result

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

Great work. Just some minor nits and it should be ready to go I think.

::: js/src/vm/SPSProfiler.cpp
@@ +214,5 @@
>      // Note: this profiler string is regexp-matched by
>      // browser/devtools/profiler/cleopatra/js/parserWorker.js.
> +
> +    // Determine if the function (if any) has an explicit or guessed name
> +    bool hasAtom = (maybeFun != nullptr && maybeFun->displayAtom() != nullptr);

Nit: this can be just: bool hasAtom = maybeFun && maybeFun->displayAtom();

@@ +223,3 @@
>      if (hasAtom) {
> +        atom = maybeFun->displayAtom()->charsZ();
> +        lenAtom = js_strlen(atom);

maybeFun->displayAtom()->length()

@@ +246,5 @@
>      if (cstr == nullptr)
>          return nullptr;
>  
> +    // Construct the descriptive string
> +    if (hasAtom) {

Nit: condition, if and else bodies all fit on one line, so no {}

@@ +247,5 @@
>          return nullptr;
>  
> +    // Construct the descriptive string
> +    if (hasAtom) {
> +        JS_snprintf(cstr, len + 1, "%hs (%s:%llu)", atom, filename, lineno);

JS_snprintf returns the length of the output (excluding the 0 at the end). Can you MOZ_ASSERT this matches |len|?

::: js/src/vm/SPSProfiler.h
@@ +128,1 @@
>                                     JSFunction *function);

(Pre-existing) nit: this fits on one line.

Comment 17

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 1 v2: Refactor allocProfileString to not require a JSContext and remove JSContext * parameters from functions that no longer require them as a result

Thanks for the comments!

(In reply to Jan de Mooij [:jandem] from comment #16)
> ::: js/src/vm/SPSProfiler.cpp
> @@ +214,5 @@
> >      // Note: this profiler string is regexp-matched by
> >      // browser/devtools/profiler/cleopatra/js/parserWorker.js.
> > +
> > +    // Determine if the function (if any) has an explicit or guessed name
> > +    bool hasAtom = (maybeFun != nullptr && maybeFun->displayAtom() != nullptr);
> 
> Nit: this can be just: bool hasAtom = maybeFun && maybeFun->displayAtom();

Done.

> @@ +223,3 @@
> >      if (hasAtom) {
> > +        atom = maybeFun->displayAtom()->charsZ();
> > +        lenAtom = js_strlen(atom);
> 
> maybeFun->displayAtom()->length()

Ah, nice - done.

> @@ +246,5 @@
> >      if (cstr == nullptr)
> >          return nullptr;
> >  
> > +    // Construct the descriptive string
> > +    if (hasAtom) {
> 
> Nit: condition, if and else bodies all fit on one line, so no {}

Done. Went through the style guide and touched up on a few other details as a result :)

> @@ +247,5 @@
> >          return nullptr;
> >  
> > +    // Construct the descriptive string
> > +    if (hasAtom) {
> > +        JS_snprintf(cstr, len + 1, "%hs (%s:%llu)", atom, filename, lineno);
> 
> JS_snprintf returns the length of the output (excluding the 0 at the end).
> Can you MOZ_ASSERT this matches |len|?

Done.

> ::: js/src/vm/SPSProfiler.h
> @@ +128,1 @@
> >                                     JSFunction *function);
> 
> (Pre-existing) nit: this fits on one line.

Done, also fixed another one like this.

Comment 18

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2 v6: Add locks around |strings| table access and profiler enabled state to avoid races

(In reply to Jan de Mooij [:jandem] from comment #13)
> Another problem: MIRGenerator::instrumentedProfiling() checks if SPS is
> enabled, this could race if the profiler is enabled/disabled on the main
> thread while we're compiling.
> 
> The simplest fix is to cancel all off-thread compilations whenever SPS is
> enabled/disabled (maybe we're doing that already, didn't check).

SPSProfiler::enable calls ReleaseAllJITCode, which calls jit::InvalidateAll, which calls StopAllOffThreadCompilations, so I think we should be good as far as off-thread compilations are concerned. However if compilations can be triggered from a worker, SPSProfiler::enabled could still race with SPSProfiler::enable, so I added the lock to both. Performance doesn't seem to be measurably impacted.

Comment 19

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 4: Allow parallel compilation while the SPS Profiler is active

Split this off into its own patch because it made testing easier. Note the second !cx->runtime()->spsProfiler.enabled() check was added by bhackett for bug 785905 - I don't think there should be any reason for it after parts 1 and 2 though.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8364476 [details] [diff] [review]
Part 1 v2: Refactor allocProfileString to not require a JSContext and remove JSContext * parameters from functions that no longer require them as a result

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

Thanks, nice work!

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 8364480 [details] [diff] [review]
Part 2 v6: Add locks around |strings| table access and profiler enabled state to avoid races

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

You don't have to lock for enabled(): only the main thread can enable the profiler, and SPSProfiler::enable will cancel any off-thread compilations first. DOM worker threads have their own JSRuntime, SPSProfiler etc.

::: js/src/vm/SPSProfiler.cpp
@@ +93,5 @@
>  const char*
>  SPSProfiler::profileString(JSScript *script, JSFunction *maybeFun)
>  {
>      JS_ASSERT(strings.initialized());
> +    SPSAutoLock lock(lock_);

Is it ok for strings.initialized() to race? it seems safer to move this line above the assert.

@@ +119,5 @@
>       * done.
>       */
>      if (!strings.initialized())
>          return;
> +    SPSAutoLock lock(lock_);

Shouldn't we move this above the if (!strings.initialized()) line?

::: js/src/vm/SPSProfiler.h
@@ +198,5 @@
>  /*
> + * This class is used to make sure the strings table
> + * is only accessed on one thread at a time.
> + */
> +class SPSAutoLock

Nit: convention is for the names of these RAII classes to start with Auto, like AutoLockGC, AutoLockForOperationCallback etc. So maybe AutoLockSPS or AutoLockSPSStrings?

@@ +205,5 @@
> +#ifdef JS_THREADSAFE
> +    SPSAutoLock(PRLock *lock)
> +    {
> +        lock_ = lock;
> +        if (lock)

Can't we MOZ_ASSERT(lock); here instead?

@@ +218,5 @@
> +    SPSAutoLock(PRLock *) {}
> +#endif
> +
> +  private:
> +    SPSAutoLock() {}

Nit: can remove this line; if the class has a non-default constructor there won't be a default constructor like this.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8364484 [details] [diff] [review]
Part 4: Allow parallel compilation while the SPS Profiler is active

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

This patch itself is fine, but there's something we should fix first: Lowering.cpp does runtime->spsProfiler().slowAssertionsEnabled() but it's possible slow assertions will be enabled on the main thread between the Lowering and CodeGenerator phases on the background thread and bad things could happen.

How about this: add a bool to JitCompileOptions (a new class that was added a few days ago), spsSlowAssertionsEnabled_. JitCompileOptions is initialized on the main thread, so we don't have to worry about races there. Then we can check that in Lowering.cpp and CodeGenerator.cpp. You can also remove IonInstrumentation::slowAssertions().

Comment 23

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 3: Move SPSProfiler::slowAssertionsEnabled check in LIRGenerator::visitFunctionBoundary to JitCompileOptions

I think this should do it. Turning this into part 3 since it's another prerequisite :)

Comment 24

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8364484 [details] [diff] [review]
Part 4: Allow parallel compilation while the SPS Profiler is active

># HG changeset patch
># Parent fb67611a87706a88e61ea987bac77fbb883eda7a
># User Emanuel Hoogeveen <[e-mail]>
>Bug 841646 - Part 4: Allow parallel compilation while the SPS Profiler is active
>
>diff --git a/js/src/jit/Ion.cpp b/js/src/jit/Ion.cpp
>--- a/js/src/jit/Ion.cpp
>+++ b/js/src/jit/Ion.cpp
>@@ -1589,24 +1589,19 @@ OffThreadCompilationAvailable(JSContext 
> {
>     // Even if off thread compilation is enabled, compilation must still occur
>     // on the main thread in some cases. Do not compile off thread during an
>     // incremental GC, as this may trip incremental read barriers.
>     //
>     // Skip off thread compilation if PC count profiling is enabled, as
>     // CodeGenerator::maybeCreateScriptCounts will not attach script profiles
>     // when running off thread.
>-    //
>-    // Also skip off thread compilation if the SPS profiler is enabled, as it
>-    // stores strings in the spsProfiler data structure, which is not protected
>-    // by a lock.
>     return cx->runtime()->canUseParallelIonCompilation()
>         && cx->runtime()->gcIncrementalState == gc::NO_INCREMENTAL
>-        && !cx->runtime()->profilingScripts
>-        && !cx->runtime()->spsProfiler.enabled();
>+        && !cx->runtime()->profilingScripts;
> }
> 
> static void
> TrackAllProperties(JSContext *cx, JSObject *obj)
> {
>     JS_ASSERT(obj->hasSingletonType());
> 
>     for (Shape::Range<NoGC> range(obj->lastProperty()); !range.empty(); range.popFront())
>@@ -1752,18 +1747,17 @@ IonCompile(JSContext *cx, JSScript *scri
>         // enabled when offthread compilation is disabled. So don't watch for
>         // proper use of the compilation lock.
>         ionCompiling.construct();
>     }
> 
>     Maybe<AutoProtectHeapForIonCompilation> protect;
>     if (js_JitOptions.checkThreadSafety &&
>         cx->runtime()->gcIncrementalState == gc::NO_INCREMENTAL &&
>-        !cx->runtime()->profilingScripts &&
>-        !cx->runtime()->spsProfiler.enabled())
>+        !cx->runtime()->profilingScripts)
>     {
>         protect.construct(cx->runtime());
>     }
> 
>     IonSpewNewFunction(graph, builderScript);
> 
>     bool succeeded = builder->build();
>     builder->clearForBackEnd();

Comment 25

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2 v7: Add locks around |strings| table access to avoid races

Well, I'm just doing things all out of order today.

(In reply to Jan de Mooij [:jandem] from comment #21)
> You don't have to lock for enabled(): only the main thread can enable the
> profiler, and SPSProfiler::enable will cancel any off-thread compilations
> first. DOM worker threads have their own JSRuntime, SPSProfiler etc.

Great, done.

> ::: js/src/vm/SPSProfiler.cpp
> @@ +93,5 @@
> >  const char*
> >  SPSProfiler::profileString(JSScript *script, JSFunction *maybeFun)
> >  {
> >      JS_ASSERT(strings.initialized());
> > +    SPSAutoLock lock(lock_);
> 
> Is it ok for strings.initialized() to race? it seems safer to move this line
> above the assert.
> 
> @@ +119,5 @@
> >       * done.
> >       */
> >      if (!strings.initialized())
> >          return;
> > +    SPSAutoLock lock(lock_);
> 
> Shouldn't we move this above the if (!strings.initialized()) line?

Done. I was using strings.initialized() as a proxy to see if the lock was initialized, since that was happening in setProfilingStack. But this was just an artifact of an earlier revision: I've moved initialization of the lock into the constructor, so it should never legally be null now.

> ::: js/src/vm/SPSProfiler.h
> @@ +198,5 @@
> >  /*
> > + * This class is used to make sure the strings table
> > + * is only accessed on one thread at a time.
> > + */
> > +class SPSAutoLock
> 
> Nit: convention is for the names of these RAII classes to start with Auto,
> like AutoLockGC, AutoLockForOperationCallback etc. So maybe AutoLockSPS or
> AutoLockSPSStrings?

Done.

> @@ +205,5 @@
> > +#ifdef JS_THREADSAFE
> > +    SPSAutoLock(PRLock *lock)
> > +    {
> > +        lock_ = lock;
> > +        if (lock)
> 
> Can't we MOZ_ASSERT(lock); here instead?

Done. I removed this in v6 because the locking in enable() and enabled() was happening before the lock was initialized; that can't happen anymore now.

> @@ +218,5 @@
> > +    SPSAutoLock(PRLock *) {}
> > +#endif
> > +
> > +  private:
> > +    SPSAutoLock() {}
> 
> Nit: can remove this line; if the class has a non-default constructor there
> won't be a default constructor like this.

Nice, done.

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 8365051 [details] [diff] [review]
Part 2 v7: Add locks around |strings| table access to avoid races

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

Nice! r=me.

::: js/src/vm/SPSProfiler.h
@@ +204,5 @@
> +  public:
> +#ifdef JS_THREADSAFE
> +    AutoSPSLock(PRLock *lock)
> +    {
> +        MOZ_ASSERT(lock, "Parameter should not be null!");

Nit: just MOZ_ASSERT(lock); -- the comment doesn't add anything.

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 8365049 [details] [diff] [review]
Part 3: Move SPSProfiler::slowAssertionsEnabled check in LIRGenerator::visitFunctionBoundary to JitCompileOptions

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

Nice, r=me with the changes requested below.

::: js/src/jit/Lowering.cpp
@@ +3274,5 @@
>      if (!add(lir, ins))
>          return false;
>      // If slow assertions are enabled, then this node will result in a callVM
>      // out to a C++ function for the assertions, so we will need a safepoint.
> +    return !gen->options.spsSlowAssertionsEnabled() || assignSafepoint(lir, ins);

You should also use this instead of sps_.slowAssertions() in CodeGenerator.cpp, and please remove SPSInstrumentation::slowAssertions() in SPSProfiler.h

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8364484 [details] [diff] [review]
Part 4: Allow parallel compilation while the SPS Profiler is active

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

Great work!

Comment 29

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8365051 [details] [diff] [review]
Part 2 v7: Add locks around |strings| table access to avoid races

># HG changeset patch
># Parent ecd05f83ee785b5afaac2942a3fd284378051f0b
># User Emanuel Hoogeveen <[e-mail]>
>Bug 841646 - Part 2: Add locks around |strings| table access to avoid races
>
>diff --git a/js/src/vm/SPSProfiler.cpp b/js/src/vm/SPSProfiler.cpp
>--- a/js/src/vm/SPSProfiler.cpp
>+++ b/js/src/vm/SPSProfiler.cpp
>@@ -23,29 +23,38 @@ SPSProfiler::SPSProfiler(JSRuntime *rt)
>   : rt(rt),
>     stack_(nullptr),
>     size_(nullptr),
>     max_(0),
>     slowAssertions(false),
>     enabled_(false)
> {
>     JS_ASSERT(rt != nullptr);
>+#ifdef JS_THREADSAFE
>+    lock_ = PR_NewLock();
>+    if (lock_ == nullptr)
>+        MOZ_CRASH("Couldn't allocate lock!");
>+#endif
> }
> 
> SPSProfiler::~SPSProfiler()
> {
>     if (strings.initialized()) {
>         for (ProfileStringMap::Enum e(strings); !e.empty(); e.popFront())
>             js_free(const_cast<char *>(e.front().value()));
>     }
>+#ifdef JS_THREADSAFE
>+    PR_DestroyLock(lock_);
>+#endif
> }
> 
> void
> SPSProfiler::setProfilingStack(ProfileEntry *stack, uint32_t *size, uint32_t max)
> {
>+    AutoSPSLock lock(lock_);
>     JS_ASSERT_IF(size_ && *size_ != 0, !enabled());
>     if (!strings.initialized())
>         strings.init();
>     stack_ = stack;
>     size_  = size;
>     max_   = max;
> }
> 
>@@ -74,16 +83,17 @@ SPSProfiler::enable(bool enabled)
>     jit::ToggleBaselineSPS(rt, enabled);
> #endif
> }
> 
> /* Lookup the string for the function/script, creating one if necessary */
> const char*
> SPSProfiler::profileString(JSScript *script, JSFunction *maybeFun)
> {
>+    AutoSPSLock lock(lock_);
>     JS_ASSERT(strings.initialized());
>     ProfileStringMap::AddPtr s = strings.lookupForAdd(script);
>     if (s)
>         return s->value();
>     const char *str = allocProfileString(script, maybeFun);
>     if (str == nullptr)
>         return nullptr;
>     if (!strings.add(s, script, str)) {
>@@ -98,16 +108,17 @@ SPSProfiler::onScriptFinalized(JSScript 
> {
>     /*
>      * This function is called whenever a script is destroyed, regardless of
>      * whether profiling has been turned on, so don't invoke a function on an
>      * invalid hash set. Also, even if profiling was enabled but then turned
>      * off, we still want to remove the string, so no check of enabled() is
>      * done.
>      */
>+    AutoSPSLock lock(lock_);
>     if (!strings.initialized())
>         return;
>     if (ProfileStringMap::Ptr entry = strings.lookup(script)) {
>         const char *tofree = entry->value();
>         strings.remove(entry);
>         js_free(const_cast<char *>(tofree));
>     }
> }
>diff --git a/js/src/vm/SPSProfiler.h b/js/src/vm/SPSProfiler.h
>--- a/js/src/vm/SPSProfiler.h
>+++ b/js/src/vm/SPSProfiler.h
>@@ -7,16 +7,17 @@
> #ifndef vm_SPSProfiler_h
> #define vm_SPSProfiler_h
> 
> #include "mozilla/DebugOnly.h"
> #include "mozilla/GuardObjects.h"
> 
> #include <stddef.h>
> 
>+#include "jslock.h"
> #include "jsscript.h"
> 
> #include "js/ProfilingStack.h"
> 
> /*
>  * SPS Profiler integration with the JS Engine
>  * https://developer.mozilla.org/en/Performance/Profiling_with_the_Built-in_Profiler
>  *
>@@ -118,16 +119,17 @@ class SPSProfiler
> 
>     JSRuntime            *rt;
>     ProfileStringMap     strings;
>     ProfileEntry         *stack_;
>     uint32_t             *size_;
>     uint32_t             max_;
>     bool                 slowAssertions;
>     uint32_t             enabled_;
>+    PRLock               *lock_;
> 
>     const char *allocProfileString(JSScript *script, JSFunction *function);
>     void push(const char *string, void *sp, JSScript *script, jsbytecode *pc);
>     void pop();
> 
>   public:
>     SPSProfiler(JSRuntime *rt);
>     ~SPSProfiler();
>@@ -180,25 +182,62 @@ class SPSProfiler
> 
>     jsbytecode *ipToPC(JSScript *script, size_t ip) { return nullptr; }
> 
>     void setProfilingStack(ProfileEntry *stack, uint32_t *size, uint32_t max);
>     const char *profileString(JSScript *script, JSFunction *maybeFun);
>     void onScriptFinalized(JSScript *script);
> 
>     /* meant to be used for testing, not recommended to call in normal code */
>-    size_t stringsCount() { return strings.count(); }
>-    void stringsReset() { strings.clear(); }
>+    size_t stringsCount();
>+    void stringsReset();
> 
>     uint32_t *addressOfEnabled() {
>         return &enabled_;
>     }
> };
> 
> /*
>+ * This class is used to make sure the strings table
>+ * is only accessed on one thread at a time.
>+ */
>+class AutoSPSLock
>+{
>+  public:
>+#ifdef JS_THREADSAFE
>+    AutoSPSLock(PRLock *lock)
>+    {
>+        MOZ_ASSERT(lock);
>+        lock_ = lock;
>+        PR_Lock(lock);
>+    }
>+    ~AutoSPSLock() { PR_Unlock(lock_); }
>+#else
>+    AutoSPSLock(PRLock *) {}
>+#endif
>+
>+  private:
>+    PRLock *lock_;
>+};
>+
>+inline size_t
>+SPSProfiler::stringsCount()
>+{
>+    AutoSPSLock lock(lock_);
>+    return strings.count();
>+}
>+
>+inline void
>+SPSProfiler::stringsReset()
>+{
>+    AutoSPSLock lock(lock_);
>+    strings.clear();
>+}
>+
>+/*
>  * This class is used in RunScript() to push the marker onto the sampling stack
>  * that we're about to enter JS function calls. This is the only time in which a
>  * valid stack pointer is pushed to the sampling stack.
>  */
> class SPSEntryMarker
> {
>   public:
>     SPSEntryMarker(JSRuntime *rt

Comment 30

[Emanuel Hoogeveen [:ehoogeveen]]

Noticed the description of the patch was wrong, so I fixed that and the review nit. I wish "edit attachment as comment" showed a diff instead of quoting the whole attachment though ...

Comment 31

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 3 v2: Move SPSProfiler::slowAssertionsEnabled checks to JitCompileOptions and remove SPSInstrumentation::slowAssertions

Carrying forward r+ from jandem. Try looks green except for known bustage (B2G desktop reftests) for that revision and some random orange:

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

(In reply to Jan de Mooij [:jandem] from comment #27)
> > +    return !gen->options.spsSlowAssertionsEnabled() || assignSafepoint(lir, ins);
> 
> You should also use this instead of sps_.slowAssertions() in
> CodeGenerator.cpp, and please remove SPSInstrumentation::slowAssertions() in
> SPSProfiler.h

Done as discussed on IRC.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d52d1a8ea36c
https://hg.mozilla.org/integration/mozilla-inbound/rev/00057fa4863d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2a9d376c99
https://hg.mozilla.org/integration/mozilla-inbound/rev/a726fa7a9dbe

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d52d1a8ea36c
https://hg.mozilla.org/mozilla-central/rev/00057fa4863d
https://hg.mozilla.org/mozilla-central/rev/fd2a9d376c99
https://hg.mozilla.org/mozilla-central/rev/a726fa7a9dbe