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

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.

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

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.

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

(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 :)

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.

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.

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

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

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

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

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

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.

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

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

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

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 <emanuel.hoogeveen@gmail.com> >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();

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 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 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 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 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 <emanuel.hoogeveen@gmail.com> >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

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

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.

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

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