User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Don't close yet... submitting patches

The js::jit:ImmPtr constructor does some asserts, due to which a TLS slot is being read. However, an ImmPtr gets initialized statically with a dead pointer elsewhere, before the TLS slot is initialized. Fixed this with a special constructor for dead pointers which doesn't do these asserts.

It's not being used anywhere.

The ion jit stores the current jit context in a TLS slot. Use the mozilla::ThreadLocal infrastructure for this, and not NSPR.

The ForkJoin implementation stores the per-thread ForkJoinSlice in a TLS slot. Use the mozilla::ThreadLocal infrastructure for this, and not NSPR.

The 6 patches not marked [WIP] are ready for landing. After these go through I will break up "[WIP] js: replace NSPR usage by thin wrappers" into smaller patches and submit them for review.

Note: I asked for this bug to be filed (even tho it's a dup of bug 773491, and possibly others) because those other bug(s) have other work in them and a clean slate seems nice -- particularly for new-contributor stuff. :-)

Comment on attachment 8356278 [details] [diff] [review] js jit: fix non-standard #if preprocessor expression Review of attachment 8356278 [details] [diff] [review]: ----------------------------------------------------------------- Clearly desirable -- we've made this sort of fix before, actually. (I hope you're working against trunk and not against slightly-older code where this has already been fixed...) Regarding patch message -- the typical convention is a line like so: Bug 956899 - Use "||" as a preprocessor operator rather than the non-standard "or". r=jwalden That is, "Bug X - " followed by a quick description of what's done as a sentence, followed by "r=foo" for whoever foo is -- usually some sort of nickname. Obviously you can't quite follow this pattern when uploading patches, because you don't know the bug number initially, and you don't know who will review the patch ultimately. (Although you can certainly guess and prefill.) Just noting it so you're aware of it, particularly if you post enough patches to ever get commit access yourself.

Oh, and if you think a longer explanation is desirable, you could put it on separate lines afterward in the patch message. jimb is particularly fond of doing this. But most people generally don't do so, because the assumption is if you want to know what's up, and the patch is insufficiently explanatory, you'll look through the bug and figure it out that way.

Comment on attachment 8356279 [details] [diff] [review] js: fix compilation failure when JS_GC_ZEAL isn't defined Review of attachment 8356279 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgcinlines.h @@ +426,5 @@ > kind == FINALIZE_STRING || > kind == FINALIZE_SHORT_STRING || > kind == FINALIZE_JITCODE); > JS_ASSERT(!rt->isHeapBusy()); > JS_ASSERT(!rt->noGCOrAllocationCheck); I believe we want to perform all these assertions even when !JS_GC_ZEAL. What we want here, really, is to further condition this to account for both the if-asserting case *and* for the if-zeal cases. So this looks like it should instead be #if defined(JS_GC_ZEAL) || defined(DEBUG) JSRuntime *rt = ncx->runtime(); #endif But I'm not super-familiar with all the GC bits, so maybe there's some hidden dependency in these assertions that does require JS_GC_ZEAL. (I didn't see any looking at the method definitions, but there could be something further afield.) CCing terrence to take over this review.

Comment on attachment 8356279 [details] [diff] [review] js: fix compilation failure when JS_GC_ZEAL isn't defined Review of attachment 8356279 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: js/src/jsgcinlines.h @@ +427,5 @@ > kind == FINALIZE_SHORT_STRING || > kind == FINALIZE_JITCODE); > JS_ASSERT(!rt->isHeapBusy()); > JS_ASSERT(!rt->noGCOrAllocationCheck); > +#endif DEBUG implies JS_GC_ZEAL, but I guess there is some interaction with MOZ_DEBUG. I guess the clearest would be a combination of the two patches: #if defined(JS_GC_ZEAL) || defined(DEBUG) JSRuntime *rt = ncx->runtime(); JS_ASSERT_IF(rt...) ... #endif

(In reply to Bert from comment #3) > Created attachment 8356279 [details] [diff] [review] > js: fix compilation failure when JS_GC_ZEAL isn't defined <terrence> #if defined(DEBUG) || defined(JS_GC_ZEAL) and move the $endif below the assertions that use |rt|

Comment on attachment 8356280 [details] [diff] [review] js: avoid read from uninitialized TLS slot in debug mode Review of attachment 8356280 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, we should get rid of IonTLSInitialized, convert this to a mozilla::ThreadLocal, then use initialized() as guard on the relevant thread-local data. Then there's no need to make this change at all.

Comment on attachment 8356281 [details] [diff] [review] js shell: don't track the stack base address Review of attachment 8356281 [details] [diff] [review]: ----------------------------------------------------------------- Lovely.

Comment on attachment 8356283 [details] [diff] [review] js ion: use mozilla::ThreadLocal instead of NSPR Review of attachment 8356283 [details] [diff] [review]: ----------------------------------------------------------------- As mentioned, this wants to be changed to check ThreadLocal::initialized(). Also, we really should just make this code all the same regardless of JS_THREADSAFE. We should just use ThreadLocal in all configurations for this and avoid the different code for the different versions. If non-threadsafe is slightly slower, I don't think we care. (Particularly with non-threadsafe going away at some point.)

Comment on attachment 8356285 [details] [diff] [review] js vm: use mozilla::ThreadLocal instead of NSPR Review of attachment 8356285 [details] [diff] [review]: ----------------------------------------------------------------- Broadly fine, but I want to take the opportunity here to make this a ThreadLocal in all cases, not just the threadsafe ones, for the simplicity reasons. Also there's some slight style-tweaking in terms of names to be done. I'll pick up both when landing this. ::: js/src/vm/ForkJoin.cpp @@ +25,5 @@ > #endif // DEBUG && THREADSAFE && ION > > #include "vm/Interpreter-inl.h" > > +using mozilla::ThreadLocal; I think we might put using of particular identifiers below using of namespaces, maybe. So this should be down below. @@ +1768,5 @@ > > bool > ForkJoinSlice::InitializeTLS() > { > if (!TLSInitialized) { This boolean should similarly be replaced with TLSForkJoinSlice.initialized().

Comment on attachment 8356287 [details] [diff] [review] [WIP] js: fix the standalone vs2010 build Review of attachment 8356287 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/TypeDecls.h @@ +17,5 @@ > #ifndef js_TypeDecls_h > #define js_TypeDecls_h > > +#include "mozilla/Char16.h" > + Also not needed, for the RequiredDefine.h reasons mentioned elsewhere. ::: mfbt/Char16.h @@ +17,5 @@ > */ > > +#if defined(_MSC_VER) && (_MSC_VER > 1500) > +# include <stdint.h> > +#elif defined(_MSC_VER) Making this header define char16_t on Windows was deliberate, so this change isn't wanted. (Admittedly, trying to define char16_t on Windows is really difficult, and potentially ill-advised. But it's what we've chosen to do, for better or worse.) ::: mfbt/TypeTraits.h @@ +17,5 @@ > > #include <wchar.h> > > +#include "mozilla/Char16.h" > + Char16.h is included in js/public/RequiredDefines.h, which is required to be in the command line when using JS headers. (Typically there should be an -include js/RequiredDefines.h in the compiler invocation, first thing, or similar. On *nix this is part of the CXXFLAGS spit out by js-config. There's not a similar standard mechanism for Windows, sadly, but the requirement stands there nonetheless.) So we shouldn't need to include this here. With char16_t being unconditionally defined, we should add the right specializations for it down below at some point, rather than saying those specializations have explicitly undefined behavior. But that doesn't need to happen here, just noting it in passing.

Thanks for doing this! We have a lot of IonContext TLS lookups and the NSPR implementation is really slow. I eliminated most of these lookups for that reason (bug 937540) but there's still a good number of them left.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf438acafb72 (preprocessor ors) https://hg.mozilla.org/integration/mozilla-inbound/rev/2383c11de7c1 (zeal compilation) https://hg.mozilla.org/integration/mozilla-inbound/rev/12cf6a6c5c9a (stack base address) https://hg.mozilla.org/integration/mozilla-inbound/rev/5735c0b01f19 (ThreadLocal in vm)

https://hg.mozilla.org/mozilla-central/rev/bf438acafb72 https://hg.mozilla.org/mozilla-central/rev/2383c11de7c1 https://hg.mozilla.org/mozilla-central/rev/12cf6a6c5c9a https://hg.mozilla.org/mozilla-central/rev/5735c0b01f19

Y'all gotta love the smell of fresh patches in the morning.

Comment on attachment 8362313 [details] [diff] [review] js: Add wrapper for posix and windows condition variables. r=terrence Review of attachment 8362313 [details] [diff] [review]: ----------------------------------------------------------------- * Oops. USEC_PER_MSEC is wrong.

Comment on attachment 8362315 [details] [diff] [review] js: Add wrapper for posix and windows threads. r=terrence Review of attachment 8362315 [details] [diff] [review]: ----------------------------------------------------------------- * in threading/posix/Thread.cpp, there is no need to include Once.h.

Comment on attachment 8362312 [details] [diff] [review] js: Add wrapper for posix and windows mutexes. r=terrence Review of attachment 8362312 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/threading/posix/MutexPlatformData.h @@ +3,5 @@ > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef platform_win_MutexPlatformData_h should be platform_posix_MutexPlatformData_h

Comment on attachment 8362313 [details] [diff] [review] js: Add wrapper for posix and windows condition variables. r=terrence Review of attachment 8362313 [details] [diff] [review]: ----------------------------------------------------------------- * Oops. USEC_PER_MSEC is wrong. ::: js/src/threading/windows/ConditionVariable.cpp @@ +303,5 @@ > + // broadcast, clear the wakeup mode and reset the wake-all event. > + // A race condition similar to the case described above could > + // occur, so waitResult could be WAIT_TIMEOUT, but that doesn't > + // matter for the actions that need to be taken. > + assert(waitResult = WAIT_OBJECT_0 + 1 || double space

If you want to support setting a stack size on Windows, you should probably pass in STACK_SIZE_PARAM_IS_A_RESERVATION to _beginthreadex: see bug 958796. It would be nice to have this ability - bhackett set the stack size for workers in bug 942984, but had to exclude Windows in bug 943924, and I think the reason is that NSPR doesn't use that flag.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #42) > If you want to support setting a stack size on Windows, you should probably > pass in STACK_SIZE_PARAM_IS_A_RESERVATION to _beginthreadex: see bug 958796. > It would be nice to have this ability - bhackett set the stack size for > workers in bug 942984, but had to exclude Windows in bug 943924, and I think > the reason is that NSPR doesn't use that flag. Right now the new threading API has no support for setting the stack size whatsoever. I will add that ability after this round of reviews.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #42) > If you want to support setting a stack size on Windows, you should probably > pass in STACK_SIZE_PARAM_IS_A_RESERVATION to _beginthreadex: see bug 958796. > It would be nice to have this ability - bhackett set the stack size for > workers in bug 942984, but had to exclude Windows in bug 943924, and I think > the reason is that NSPR doesn't use that flag. Right now the new threading API has no support for setting the stack size whatsoever. I will add that ability after this round of reviews.

Comment on attachment 8362315 [details] [diff] [review] js: Add wrapper for posix and windows threads. r=terrence Review of attachment 8362315 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/threading/Thread.h @@ +27,5 @@ > + { }; > + > + ~Thread(); > + > + bool start(Entry entry, void* arg); Needs an additional argument for specifying the stack size. See https://bugzilla.mozilla.org/show_bug.cgi?id=956899#c42

Comment on attachment 8362315 [details] [diff] [review] js: Add wrapper for posix and windows threads. r=terrence Review of attachment 8362315 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/threading/posix/Thread.cpp @@ +124,5 @@ > + > + if (pthread_join(platformData()->ptThread, NULL) != 0) > + abort(); > + > + id_ = none(); move this to before pthread_join @@ +139,5 @@ > +} > + > + > +Thread::~Thread() { > + // In theory we could just CloseHandle(handle) here if the thread is still this is not windows @@ +186,5 @@ > + int r = pthread_setname_np_dld(name); > + return r == 0; > + > +#elif defined(__FreeBSD__) || defined(__OpenBSD__) > + int r = pthread_set_name_np(pthread_self(), name); need to include pthread_np.h ::: js/src/threading/windows/Thread.cpp @@ +75,5 @@ > + delete trampoline; > + return false; > + } > + > + data->handle = (HANDLE) handle; c-style cast

Comment on attachment 8362312 [details] [diff] [review] js: Add wrapper for posix and windows mutexes. r=terrence Review of attachment 8362312 [details] [diff] [review]: ----------------------------------------------------------------- This is excellent! Just a handful of stylistic nits to address to bring the style inline with SpiderMonkey's expectations. You may also want to skim https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style to make sure I didn't miss anything. I'd also definitely run |make check| against your full series, as that runs a program to verify the #include ordering mechanically and I'm not 100% sure what our exact rules are for this case. ::: js/src/threading/Mutex.h @@ +10,5 @@ > +#include <stdint.h> > + > +#include "mozilla/Attributes.h" > + > + Only one newline. @@ +13,5 @@ > + > + > +namespace js { > + > +class Mutex { Brace on new line. @@ +17,5 @@ > +class Mutex { > + public: > + struct PlatformData; > + > + inline Mutex() Class methods with a body declared are implicitly |inline|, so please remove the manual declaration. @@ +31,5 @@ > + > + private: > + // Disallow copy and assign. > + Mutex(const Mutex& that) MOZ_DELETE; > + void operator=(const Mutex& that) MOZ_DELETE; Please don't name the unused parameters here. @@ +38,5 @@ > + > + bool initialized_; > + > + // Linux and maybe other platforms define the storage size of > + // pthread_mutex_t in bytes. However we must define it as an array of void Comma after the introductory preposition, "however." ::: js/src/threading/posix/Mutex.cpp @@ +5,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "threading/Mutex.h" > + > +#include <assert.h> This should use #include <mozilla/Assertions.h> and then MOZ_ASSERT() instead of assert(). @@ +8,5 @@ > + > +#include <assert.h> > +#include <pthread.h> > + > +#include "threading/posix/MutexPlatformData.h" Move this #include to right below #include "threading/Mutex.h" to ensure that it isn't bootlegging any of the intervening #includes. @@ +11,5 @@ > + > +#include "threading/posix/MutexPlatformData.h" > + > + > +namespace js { Remove the namespace declaration: style for cpp files is to put the namespace into each declaration, rather than wrapping. So: bool js::Mutex::initialize() { @@ +13,5 @@ > + > + > +namespace js { > + > +bool Mutex::initialize() { Type, name, and brace all go on separate lines. @@ +14,5 @@ > + > +namespace js { > + > +bool Mutex::initialize() { > + assert(!initialized_); MOZ_ASSERT(!initialized_); @@ +24,5 @@ > + return true; > +} > + > + > +void Mutex::lock() { Type, name, and brace all go on separate lines. And start the method with |MOZ_ASSERT(initialized_);|. @@ +26,5 @@ > + > + > +void Mutex::lock() { > + int r = pthread_mutex_lock(&platformData()->ptMutex); > + assert(r == 0); MOZ_ASSERT(r == 0); @@ +30,5 @@ > + assert(r == 0); > +} > + > + > +void Mutex::unlock() { Type, name, and brace all go on separate lines. And start the method with |MOZ_ASSERT(initialized_);|. @@ +32,5 @@ > + > + > +void Mutex::unlock() { > + int r = pthread_mutex_unlock(&platformData()->ptMutex); > + assert(r == 0); MOZ_ASSERT(r == 0); @@ +36,5 @@ > + assert(r == 0); > +} > + > + > +Mutex::~Mutex() { Brace on newline. @@ +45,5 @@ > + assert(r == 0); > +} > + > + > +Mutex::PlatformData* Mutex::platformData() { Type, name, and brace all go on separate lines. @@ +46,5 @@ > +} > + > + > +Mutex::PlatformData* Mutex::platformData() { > + static_assert(sizeof platformData_ >= sizeof(PlatformData), Please use the parenthesized form of sizeof() for both of these, or at least make them consistent. ::: js/src/threading/posix/MutexPlatformData.h @@ +10,5 @@ > +#include <pthread.h> > + > +#include "threading/Mutex.h" > + > + One newline only. @@ +13,5 @@ > + > + > +namespace js { > + > +struct Mutex::PlatformData { Brace on newline. ::: js/src/threading/windows/Mutex.cpp @@ +3,5 @@ > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + One newline only. @@ +8,5 @@ > +#include "threading/Mutex.h" > + > +#include <assert.h> > + > +#include <Windows.h> Use #include "jswin.h". This wrapper will undef a bunch of symbols that both windows and the js engine define. Probably not a huge issue in this case since it should be limited to this translation unit, but no point in being inconsistent. @@ +12,5 @@ > +#include <Windows.h> > + > +#include "threading/windows/MutexPlatformData.h" > + > + One newline only. The rest of the commentary is the same as the other Mutex.cpp, please apply it all here as well. @@ +37,5 @@ > + > + > +Mutex::~Mutex() { > + if (initialized_) > + DeleteCriticalSection(&platformData()->cs); Please be consistent with the other destructor and the rest of spidermonkey and use early-exit style guarding. e.g. if (!initialized_) return; DeleteCriticalSection(&platformData()->cs); ::: js/src/threading/windows/MutexPlatformData.h @@ +10,5 @@ > +#include <Windows.h> > + > +#include "threading/Mutex.h" > + > + One newline only. @@ +13,5 @@ > + > + > +namespace js { > + > +struct Mutex::PlatformData { Brace on newline. @@ +14,5 @@ > + > +namespace js { > + > +struct Mutex::PlatformData { > + CRITICAL_SECTION cs; Please spell this out as criticalSection; no making this more cryptic than it needs to be.

Comment on attachment 8362315 [details] [diff] [review] js: Add wrapper for posix and windows threads. r=terrence Review of attachment 8362315 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/threading/posix/Thread.cpp @@ +186,5 @@ > + int r = pthread_setname_np_dld(name); > + return r == 0; > + > +#elif defined(__FreeBSD__) || defined(__OpenBSD__) > + int r = pthread_set_name_np(pthread_self(), name); pthread_set_name_np() returns |void| on DragonFly, FreeBSD and OpenBSD. Similar issue exists in NSPR, see bug 782111.

Comment on attachment 8362310 [details] [diff] [review] js: Use "&&" as a preprocessor operator rather than the non-standard "and". r=jwalden Review of attachment 8362310 [details] [diff] [review]: ----------------------------------------------------------------- I landed the previous patch with the tweaks I wanted, so we're good, no more patching needed.

Comment on attachment 8362310 [details] [diff] [review] js: Use "&&" as a preprocessor operator rather than the non-standard "and". r=jwalden Already fixed in https://hg.mozilla.org/mozilla-central/rev/9a84eebfcd41

Comment on attachment 8362311 [details] [diff] [review] js: Use mozilla::ThreadLocal instead of NSPR for IonContext's thread-local variable. r=jwalden Review of attachment 8362311 [details] [diff] [review]: ----------------------------------------------------------------- Looks broadly good, modulo style nits. I'll fix those and post a second round on the patch. I want us to get rid of the JS_THREADSAFE forking around the IonContext methods, and that gets slightly complicated enough I want to have someone who understands IonContext stuff look over it, so I'll take it from here. Thanks! ::: js/src/jit/Ion.cpp @@ +47,5 @@ > #include "jsgcinlines.h" > #include "jsinferinlines.h" > #include "jsobjinlines.h" > > +using mozilla::ThreadLocal; Goes below the |using mozilla::Maybe;| further down. @@ +66,5 @@ > { > + if (TLSIonContext.initialized()) > + return TLSIonContext.get(); > + else > + return nullptr; SpiderMonkey style never has |if (...) return ...; else return ...;| because the |else| there is meaningless -- if the if fails, control will pass along to the return regardless whether the if's there. Also, it's usually better to have exceptional cases handled first/indented. So I'd have |if (!initialized) return nullptr;| then the .get() as a separate, unindented return. @@ +158,5 @@ > bool > jit::InitializeIon() > { > #ifdef JS_THREADSAFE > if (!IonTLSInitialized) { No need for this variable at all, it's identical to |TLSIonContext.initialized()|.

I'd like this converted to a system similar to what ForkJoin uses, where the ThreadLocal is a private static class field, at some point. Also, making, say, current() always return a non-null thing and getCurrent() be maybe non-null, would be more in line with SpiderMonkey style. But smaller changes at a time. On the point of thread locals, we probably should also consider having fewer thread-locals in SpiderMonkey overall, and sticking all these individual things in one struct stored as thread-local instead. Right now every ThreadLocal is its own balkanized thing. I have no idea which ones can be initialized while other ones can't, and I have no idea whether any of them are duplicative of functionality of the others (such that two could be combined into one). Making initialization of thread-local stuff more coherent would be great. But as I said, smaller bits. For now, just make this a ThreadLocal and leave all API/organization cleanup to later.

Comment on attachment 8363862 [details] [diff] [review] Use ThreadLocal to store IonContext Review of attachment 8363862 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, great to see us get rid of more NSPR/JS_THREADSAFE stuff. (Eventually we should rename IonContext/InitializeIon to JitContext/InitializeJit but not here of course.)

Comment on attachment 8362313 [details] [diff] [review] js: Add wrapper for posix and windows condition variables. r=terrence Review of attachment 8362313 [details] [diff] [review]: ----------------------------------------------------------------- I don't see anything obviously wrong with the CV implementation, but this code is quite subtle. Please add a thorough condition variable test to jsapi-tests to exercise all of this code. ::: js/src/threading/windows/ConditionVariable.cpp @@ +42,5 @@ > +// using a static initializer. > +class ConditionVariableNativeImports { > + public: > + ConditionVariableNativeImports() { > + // Kernel32.dll is always loaded first, so this should be unfallible. s/unfallible/infallible/ @@ +95,5 @@ > + return true; > + } > + > + inline void destroy() { > + // Native condition variabled don't require cleanup. s/variabled/variables/ @@ +149,5 @@ > + > + return true; > + > + error1: > + r = CloseHandle(wakeAllEvent_); This should be closing wakeOneEvent_. @@ +183,5 @@ > + // Atomically set the wakeup mode and retrieve the number of sleepers. > + assert((sleepersCountAndWakeupMode_ & WAKEUP_MODE_MASK) == > + WAKEUP_MODE_NONE); > + uint32_t wcwm = InterlockedExchangeAdd(&sleepersCountAndWakeupMode_, > + wakeupMode); SpiderMonkey's style is to wrap at 100 chars, so all of this should fit on a single line. It's pretty clear that the sleepersCountAndWakeupMode_ update below is guarded by the semaphore. Please expand the comment on this block to explain why we need the InterlockedExchangeAdd here. @@ +234,5 @@ > + > + // Wait for either event to become signaled, which happens when > + // signal() or broadcast() is called, or for a timeout. > + HANDLE handles[2] = { wakeOneEvent_, wakeAllEvent_ }; > + DWORD waitResult = WaitForMultipleObjects(2, handles, FALSE, msec); Please note that the semaphore may or may not be held on exit here. @@ +276,5 @@ > + releaseSleepWakeupSemaphore = true; > + > + } else if (waitResult == WAIT_TIMEOUT && > + wakeupMode == WAKEUP_MODE_ONE && > + sleepersCount == 0) { Brace on newline if the if() block extends to multiple lines. @@ +309,5 @@ > + > + BOOL success = ResetEvent(wakeAllEvent_); > + assert(success); > + > + sleepersCountAndWakeupMode_ = 0 | WAKEUP_MODE_NONE; Please note why this non-interleaved update is okay. @@ +346,5 @@ > + } > + > + private: > + uint32_t sleepersCountAndWakeupMode_; > + HANDLE SleepWakeupSemaphore_; Initial letter should be lower case, like the other members. @@ +405,5 @@ > + else > + r = platformData()->fallback.wait(cs, INFINITE); > + > + if (!r) > + abort(); I'd think this should be an assertion, like the other checks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fae9d601b172 for the ThreadLocal for IonContext bit.

Comment on attachment 8362314 [details] [diff] [review] js: Add AutoMutexLock/AutoMutexUnlock helper classes for automatic locking and unlocking of a mutex within a scope. r=terrence Review of attachment 8362314 [details] [diff] [review]: ----------------------------------------------------------------- f=me ::: js/src/threading/Mutex.h @@ +56,5 @@ > #endif > }; > > + > +class AutoMutexLock { { on new line. @@ +73,5 @@ > + Mutex* mutex_; > +}; > + > + > +class AutoMutexUnlock { { on new line.

Comment on attachment 8362315 [details] [diff] [review] js: Add wrapper for posix and windows threads. r=terrence Review of attachment 8362315 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

@terrence I wrote condition variable tests and actually found issues (now fixed - spoiler: https://github.com/piscisaureus/smjs/commit/560a6cb8e92498f8bd9c540097adde2f01bf04e9#diff-1) But what I found is that * The native implementation that we use on Vista+ suffers from a lot of spurious wakeups. The pthread "standard" also explicitly allows for spurious wakeups, so that doesn't need to be an issue. But I don't know about NSPR... is it okay for ConditionVariable::wait() to wake up spuriously? * The test I wrote currently does not tolerate spurious wakeups. I can make it do that, but it becomes sort of a toothless tiger then. Any ideas on how it can remain useful? * The native implementation doesn't let us distinguish between a timeout and a wakeup (for timed waits) reliably. I would suggest to change the api so it has `void ConditionVariable::wait(Mutex mutex, uint64_t timeout)` So the return value that reveals whether a thread was woken or timed out is lost. Yay/nay? * The native API *is* much faster though (non-scientific benchmarking suggests its 4x as fast)

(In reply to Bert from comment #60) Great work! > I wrote condition variable tests and actually found issues (now fixed - > spoiler: > https://github.com/piscisaureus/smjs/commit/ > 560a6cb8e92498f8bd9c540097adde2f01bf04e9#diff-1) > > But what I found is that > > * The native implementation that we use on Vista+ suffers from a lot of > spurious wakeups. The pthread "standard" also explicitly allows for spurious > wakeups, so that doesn't need to be an issue. But I don't know about NSPR... > is it okay for ConditionVariable::wait() to wake up spuriously? Yes, absolutely. http://en.wikipedia.org/wiki/CAP_theorem > * The test I wrote currently does not tolerate spurious wakeups. I can make > it do that, but it becomes sort of a toothless tiger then. Any ideas on how > it can remain useful? Adding a test to our suite that can fail means it will fail randomly, probably many times a day. Spurious wakeups are a fact of life when using CV; just make the test exercise as much of the code as possible so that we'll know if something that affects its expected functionality breaks. If spurious wakeups become a performance issue, our performance test suite, Talos, will notify us. > * The native implementation doesn't let us distinguish between a timeout and > a wakeup (for timed waits) reliably. I would suggest to change the api so it > has > `void ConditionVariable::wait(Mutex mutex, uint64_t timeout)` > So the return value that reveals whether a thread was woken or timed out is > lost. Yay/nay? Yes please! That return has always bugged me. Either you don't care so it doesn't matter or you want precise timings so it's inadequate. > * The native API *is* much faster though (non-scientific benchmarking > suggests its 4x as fast) That's not surprising or worrisome. If we have a performance issue on XP because of CV overhead we have a /much/ bigger problem to deal with.

Note the Windows API still needs to be extended to take a stack size parameter (especially now that bug 958796 and bug 969038 have landed).

What's the status of this bug? Anything I can do to help move this forward? I'd be happy to rebase/fix/review your patches if needed. NSPR is causing weird jit-test problems on Windows, see bug 970063 comment 7. It'd be great to get this landed :)

Ditto what Jan said above.

Bert has done all the hard work already -- the least we can do is get them cleaned up and landed for him. I've rebased the first patch, applied the review feedback, and added a basic sanity-check test. I'm testing the windows side of things here: https://tbpl.mozilla.org/?tree=Try&rev=952b479542f5

Now that Steve's nspr auto-building works, the pressure to get this in is considerably lower.

(In reply to Terrence Cole [:terrence] from comment #67) > Now that Steve's nspr auto-building works, the pressure to get this in is > considerably lower. Oh, but still sooooo wanted!

Comment on attachment 8457630 [details] [diff] [review] mutex-wrapper-v1.diff 2 years on and the situation with threading in mozilla-central has not improved. It's gotten downright stupid, even. I'd like this to be a totally uncontroversial low-level threading API that we can test locally and move to mozglue and use everywhere instead of NSPR.

Comment on attachment 8457630 [details] [diff] [review] mutex-wrapper-v1.diff Review of attachment 8457630 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable. It would be superb to have replacements for condition variables, but that's a significant amount of effort on the Windows side, so one step at a time. :) ::: js/src/threading/Mutex.h @@ +24,5 @@ > + void unlock(); > + > + private: > + Mutex(const Mutex &) MOZ_DELETE; > + void operator=(const Mutex &) MOZ_DELETE; We can now use |= delete|, hooray. I would suggest deleting the moving versions of these for extra clarity. ::: js/src/threading/posix/Mutex.cpp @@ +37,5 @@ > + > +void > +js::Mutex::lock() > +{ > + MOZ_ASSERT(initialized_); These accesses to |initialized_| look like they race: T1: Call Mutex(), Mutex::initialize() // touches initialized_ T2: Call Mutex::lock() // touches initialized_ without intervening synchronization I'd like to have the constructor call the necessary initialization mechanisms and dispense with |initialized_| unless there's a good reason to split things up this way that I'm just not considering. ::: js/src/threading/windows/Mutex.cpp @@ +15,5 @@ > +js::Mutex::initialize() > +{ > + MOZ_ASSERT(!initialized_); > + > + InitializeCriticalSection(&platformData()->criticalSection); The comments around _PR_MD_INIT_LOCKS and _PR_MD_NEW_LOCK here look pertinent: http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/windows/w95cv.c#314 I don't know whether the spin count bits are necessary in practice (seems like a reasonable idea, and other important Windows-specific bits--jemalloc, TimeStamp, libevent, chromium's synchronization bits--use spin counts), but the debug object bit sounds bad. (Though not many other places try to use InitializeCriticalSectionEx, so....)

A basic, std::mutex work-alike. This is missing some of the more esoteric features like try_lock, as I'd like to avoid as much complexity as possible if we don't really need it.

Replace AutoLockForExclusiveAccess's PRLock with a Mutex.

Add a std::lock_guard work-alike. This is again a simple version that supports only a single object at a time. Seems to be enough for all of the uses within SpiderMonkey that I've run up against so far.

Replace the PRLock in JitSpewer with a Mutex.

Replace the PRLock and custom guard object in TraceLogger with a Mutex and LockGuard.

And for TraceLoggingGraph.

Comment on attachment 8725947 [details] [diff] [review] 00_mutex__00_impl-v2.diff Review of attachment 8725947 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/threading/windows/Mutex.cpp @@ +13,5 @@ > +#include "threading/windows/MutexPlatformData.h" > + > +js::Mutex::Mutex() > +{ > + mozilla::DebugOnly<BOOL> r = InitializeCriticalSection( D'oh! Forgot to change this to the ...Ex version.

Comment on attachment 8725952 [details] [diff] [review] 00_mutex__10_LockGuard-v0.diff Review of attachment 8725952 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/threading/LockGuard.h @@ +14,5 @@ > +{ > + Mutex& lock; > + > +public: > + LockGuard(Mutex& aLock) This needs an |explicit|.

Comment on attachment 8725948 [details] [diff] [review] 00_mutex__05_AutoLockForExclusiveAccess-v0.diff Review of attachment 8725948 [details] [diff] [review]: ----------------------------------------------------------------- Nice

As discussed on IRC. Rename the rust-alike Mutex to ExclusiveData and move to threading/.

Rebase ExclusiveData from PRLock to Mutex. Since Mutex initialization is now infallible, this means that so is ExclusiveData. Which means that most of ExclusiveData actually can go away now. 4 files changed, 10 insertions(+), 97 deletions(-)

Comment on attachment 8726335 [details] [diff] [review] NN_exclusivedata__00_rename-v0.diff Review of attachment 8726335 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/threading/ExclusiveData.h @@ +102,5 @@ > * guard->inc(n); > * } > * }; > * > + * The API design is based on Rust's `std::sync::ExclusiveData<T>` type. The Rust type did not change names ;)

Comment on attachment 8726343 [details] [diff] [review] 00_mutex__03_ExclusiveData-v0.diff Review of attachment 8726343 [details] [diff] [review]: ----------------------------------------------------------------- \o/ Thanks! ::: js/src/jsapi-tests/testThreadingExclusiveData.cpp @@ +68,5 @@ > } > > BEGIN_TEST(testExclusiveData) > { > + auto counter = js::ExclusiveData<uint64_t>(0); Now that this stuff is infallible, I think this looks a little better: js::ExclusiveData<uint64_t> counter(0);

Win8 builds and tests are starting to return green and the winXP cgc tests have been going over an hour, so should have passed jsapi-tests already. As we discussed on IRC, this has to take the same mostly dynamic approach that nspr uses. We're building with the win7 toolkit in order to support WinXP, so InitializeCriticalSectionEx is not even available unless we manually link to it. However, if it is available, we *have* to use that interface to keep windows from leaking the debug info it attaches to mutexes by default. It's unfortunate, but I think this really is our only option.

A std::condition_variable takes a std::unique_lock instead of a std::mutex directly in order to provide extra safety. A std::unique_lock is very much like a std::lock_guard except that it is also movable and manually {un}lockable. It's actually quite a strange beast, as the tests show.

Comment on attachment 8726369 [details] [diff] [review] 00_mutex__01_full_windows_support-v0.diff Review of attachment 8726369 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/threading/windows/Mutex.cpp @@ +27,3 @@ > js::Mutex::Mutex() > { > + if (!sInitialized) { This isn't really safe to do here, is it? Consider that two threads can be the first threads to enter the Mutex constructor, and then we have a write-after-write race on these static variables. There's also no guarantee that sInitializeCriticalSectionEx's write would be seen along with the sInitialized write, so you can still wind up with code using the old, busted way of doing things, even if we're on Vista+. (Just because some thread initializes these variables properly doesn't mean that all other threads are necessarily going to observe those writes, since there's no synchronization...) So this is a mostly-r+?

Comment on attachment 8725947 [details] [diff] [review] 00_mutex__00_impl-v2.diff Review of attachment 8725947 [details] [diff] [review]: ----------------------------------------------------------------- r=me with suggested changes below. ::: js/src/threading/posix/Mutex.cpp @@ +16,5 @@ > + > +js::Mutex::Mutex() > +{ > + DebugOnly<int> r = pthread_mutex_init(&platformData()->ptMutex, NULL); > + MOZ_ASSERT(r == 0); WDYT about making all the return value checks MOZ_RELEASE_ASSERT instead? ::: js/src/threading/windows/Mutex.cpp @@ +15,5 @@ > +js::Mutex::Mutex() > +{ > + mozilla::DebugOnly<BOOL> r = InitializeCriticalSection( > + &platformData()->criticalSection, 1500, CRITICAL_SECTION_NO_DEBUG_INFO); > + MOZ_ASSERT(r); And this one, too, for completeness.

Comment on attachment 8725952 [details] [diff] [review] 00_mutex__10_LockGuard-v0.diff Review of attachment 8725952 [details] [diff] [review]: ----------------------------------------------------------------- I don't know that I'm a fan of the name (for which I blame the standards committee, not you), but maybe it will grow on me. Maybe add: using AutoMutexLock = LockGuard<Mutex>; ?

(In reply to Nathan Froyd [:froydnj] from comment #91) > WDYT about making all the return value checks MOZ_RELEASE_ASSERT instead? Yes please! I actually went back and forth a couple times with this. On second thought I think that release asserts would be better here: it's possible for these to fail safe, but it's pretty unlikely. Besides, we should be setting the bar disproportionately higher for threading primitive code. (In reply to Nathan Froyd [:froydnj] from comment #90) > Comment on attachment 8726369 [details] [diff] [review] > 00_mutex__01_full_windows_support-v0.diff > > Review of attachment 8726369 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/threading/windows/Mutex.cpp > @@ +27,3 @@ > > js::Mutex::Mutex() > > { > > + if (!sInitialized) { > > This isn't really safe to do here, is it? Consider that two threads can be > the first threads to enter the Mutex constructor, and then we have a > write-after-write race on these static variables. Good catch! The ConditionVariable code is already using a static initializer to do this and I think that would be better here as well. I'll fix it and ask for re-review.

Comment on attachment 8726441 [details] [diff] [review] 05_UniqueLock__00_impl-v0.diff Review of attachment 8726441 [details] [diff] [review]: ----------------------------------------------------------------- I don't really get the need for this class in the standard; I've read through N2406, which I think is the proposal that introduced it: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2406.html and the rationale for unique_lock is not at all clear. "Despite the fact that scoped_lock efficiently covers most of the use cases for locking and unlocking a mutex, it does not cover all of the use cases. To cover the rest of these use cases unique_lock is introduced."...without actually discussing the use cases! It seems like it might be for significantly more exotic locks/locking algorithms than we typically use? It seems unfortunate that we have to have this baggage to make condition_variable or equivalent work. Do you understand the use of/need for this class? WDYT about making ConditionVariable just take |mutex&|? ::: js/src/threading/UniqueLock.h @@ +35,5 @@ > + } > + > + explicit UniqueLock(Mutex& aLock) > + : pm(&aLock) > + , owns(true) This is not quite right, is it? Because we're saying we hold the lock prior to the mutex actually being locked, which seems like a recipe for subtle problems to creep in. I'll note too that GCC's <mutex> contains "// XXX use atomic_bool" around unique_lock's ownership variable; we should try to use atomics from the start. @@ +94,5 @@ > + bool tmpOwns = aOther.owns; > + aOther.pm = pm; > + aOther.owns = owns; > + pm = tmpLock; > + owns = tmpOwns; Can we just use mozilla::Swap (Move.h) for all of this?

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f7f0a43f051424dbdc87db33a9947f7b87e8c89 Bug 956899 - Move and rename rust-alike Mutex to ExclusiveData; r=fitzgen

This broke builds with https://treeherder.mozilla.org/logviewer.html#?job_id=23172390&repo=mozilla-inbound Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/359ea99d2000

I applied some review commentary in the wrong patch by mistake.

(In reply to Nathan Froyd [:froydnj] from comment #94) > Comment on attachment 8726441 [details] [diff] [review] > 05_UniqueLock__00_impl-v0.diff > > Review of attachment 8726441 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't really get the need for this class in the standard; I've read > through N2406, which I think is the proposal that introduced it: > > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2406.html > > and the rationale for unique_lock is not at all clear. "Despite the fact > that scoped_lock efficiently covers most of the use cases for locking and > unlocking a mutex, it does not cover all of the use cases. To cover the rest > of these use cases unique_lock is introduced."...without actually discussing > the use cases! I think they mean things like: https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp#3035-3037 That said, I'd much prefer an UnlockGuard (LockUnguard?) that uses RAII. > It seems like it might be for significantly more exotic > locks/locking algorithms than we typically use? I LOL'd for real. Then I briefly remembered the blissful ignorance I lived in before reading js/src/vm/HelperThreads and felt honest regret. > It seems unfortunate that > we have to have this baggage to make condition_variable or equivalent work. > > Do you understand the use of/need for this class? Not really. I think the intent is to signal that CV has permission to unlock, whereas a LockGuard does not have that capability? I pretty much agree with all of that. > WDYT about making ConditionVariable just take |mutex&|? I would prefer a LockGuard to keep the type-requirement of having taken the lock before entering the CV. > ::: js/src/threading/UniqueLock.h > @@ +35,5 @@ > > + } > > + > > + explicit UniqueLock(Mutex& aLock) > > + : pm(&aLock) > > + , owns(true) > > This is not quite right, is it? Because we're saying we hold the lock prior > to the mutex actually being locked, which seems like a recipe for subtle > problems to creep in. Yeah, I agree! I think the intent is that UniqueLock is intended to be, well, unique, so there *shouldn't* be contention on |owns|. Or purpose to having owns at all. It's not even used internally. Or externally, afaict. That said, this is all sketchy enough to warrant extreme skepticism. > I'll note too that GCC's <mutex> contains "// XXX use atomic_bool" around > unique_lock's ownership variable; we should try to use atomics from the > start. e.g. Adequate Skepticism. > @@ +94,5 @@ > > + bool tmpOwns = aOther.owns; > > + aOther.pm = pm; > > + aOther.owns = owns; > > + pm = tmpLock; > > + owns = tmpOwns; > > Can we just use mozilla::Swap (Move.h) for all of this? No, we cannot. Doing so triggers the Move constructor, which appears to be buggy. Or insane? I'm not sure, but I don't want to touch it with a 10 foot pole. My only worry with changing the interface is that at some point we're probably going to want to swap this all out for std::stuff. Could we perhaps just |using UniqueLock = LockGuard;|? That would keep the type benefits of requiring a LockGuard (or equivalentish) and also allow us to port to std::condition_variable with sed.

Not opening a new bug since i see this is still being worked on and has been backed out - i saw a build failure on openbsd/amd64 with this commit: 115:11.34 /home/othersrc/mozilla-central/js/src/jsapi-tests/testThreadingExclusiveData.cpp:72:33: error: no matching constructor for initialization of 'js::ExclusiveData<uint64_t>' 115:11.34 js::ExclusiveData<uint64_t> counter(0); 115:11.39 /home/othersrc/mozilla-central/js/src/threading/ExclusiveData.h:120:5: note: candidate constructor not viable: no known conversion from 'int' to 'const js::ExclusiveData<unsigned long long>' for 1st argument 115:11.40 ExclusiveData(const ExclusiveData&) = delete; 115:11.40 /home/othersrc/mozilla-central/js/src/threading/ExclusiveData.h:145:5: note: candidate constructor not viable: no known conversion from 'int' to 'js::ExclusiveData<unsigned long long>' for 1st argument 115:11.40 ExclusiveData(ExclusiveData&& rhs) 115:11.40 /home/othersrc/mozilla-central/js/src/threading/ExclusiveData.h:124:14: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided 115:11.40 explicit ExclusiveData(U&& u, ExclusiveDataBase&& base) On openbsd, uint64_t is unsigned long long.

That's not the issue at all. Automatic conversion works fine there, it's just missing the relevant constructor because I applied a correction meant for patch 3 in the series to patch 1. As per comment 97.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b411b94f8d916f8b2bdf490fcc21d1afdf4b6daa Bug 956899 - Move and rename rust-alike Mutex to ExclusiveData; r=fitzgen

I was missing the WINAPI from the function type, so spent a good long time painstakingly verifying that this is actually working exactly as expected.

I have CV working locally in windows. This is the native impl: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6391444056de

Comment on attachment 8728492 [details] [diff] [review] 00_mutex__01_full_windows_support-v1.diff Review of attachment 8728492 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the delay in reviewing this. ::: js/src/threading/windows/Mutex.cpp @@ +44,3 @@ > js::Mutex::Mutex() > { > + const static DWORD LockSpinCount = 1500; It might be worth saying something about where we get this magical 1500 from. MSDN's page on InitializeCriticalSectionAndSpinCount indicates that the heap manager uses 4000. We could just say that we got it from NSPR?

I went with the name UnlockGuard. I don't like the lock_guard naming either, but I feel like it makes sense to stay thematically close to the std:: naming.

Apparently, I am the first to need to import something into SM from the mozglue directory.

Comment on attachment 8729084 [details] [diff] [review] 06_teach_check_spidermonkey_style_about_mozglue-v0.diff Review of attachment 8729084 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/check_spidermonkey_style.py @@ +246,5 @@ > mozalloc_inclnames.add(inclname) > > + if filename.startswith('mozglue/') and filename.endswith('.h'): > + inclname = 'mozilla/' + filename.split('/')[-1] > + mfbt_inclnames.add(inclname) This looks like it'll work, but I'll ask you to clean things up slightly here. - We have |mfbt_inclnames| and |mozalloc_inclnames| already, but I think they can be merged, and the mozglue/ names will go in the same set too. I'd call this new merged set |non_js_inclnames| to distinguish it clearly from |js_inclnames|. - Can you add a mozalloc/ example and a mozglue/ example to the comment at the top of this function?

Hey Bert, very cool to see you contributing to the Mozilla codebase! Have fun! ;-)

https://hg.mozilla.org/mozilla-central/rev/1e05c8d75716 https://hg.mozilla.org/mozilla-central/rev/e0a4ca7f68a1

https://hg.mozilla.org/integration/mozilla-inbound/rev/85b84b87c6a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/59abf823d078 https://hg.mozilla.org/integration/mozilla-inbound/rev/d91c3bce840a https://hg.mozilla.org/integration/mozilla-inbound/rev/161e40cc86c1

I was scratching my head at the current implementation. Glad it's not just me. In fact, once we make the suggested change, we suddenly have 3 blocks that are identical except for the constant string, so I abstracted over that as well.

Comment on attachment 8730306 [details] [diff] [review] 06_teach_check_includes_about_mozglue-v1.diff Review of attachment 8730306 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Don't forget to update the log message :) ::: config/check_spidermonkey_style.py @@ +227,5 @@ > # - An "inclname" is how a file is referred to in a #include statement. > # > # Examples (filename -> inclname) > # - "mfbt/Attributes.h" -> "mozilla/Attributes.h" > # - "mfbt/decimal/Decimal.h -> "mozilla/Decimal.h" Please add a mozalloc/ example and a mozglue/ example here.

Comment on attachment 8362314 [details] [diff] [review] js: Add AutoMutexLock/AutoMutexUnlock helper classes for automatic locking and unlocking of a mutex within a scope. r=terrence We are using the more standardsish LockGuard and UnlockGuard instead.

Comment on attachment 8726441 [details] [diff] [review] 05_UniqueLock__00_impl-v0.diff We are opting out of this insanity.

https://hg.mozilla.org/integration/mozilla-inbound/rev/40aa5327f83e88bf4d7e2739e6c553915c5178a3 Bug 956899 - Replace PRLock with Mutex in TraceLoggingGraph; r=h4writer https://hg.mozilla.org/integration/mozilla-inbound/rev/a0b4612ed03d61bb3f51a41c253528dabc3efaa3 Bug 956899 - Implement an RAII unlocking primitive to compliment LockGuard; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/2f4dc776c043e58c34762e0d463c7d91af0f4471 Bug 956899 - Teach check_spidermonkey_style.py about mozglue; r=njn

https://hg.mozilla.org/mozilla-central/rev/85b84b87c6a7 https://hg.mozilla.org/mozilla-central/rev/59abf823d078 https://hg.mozilla.org/mozilla-central/rev/d91c3bce840a https://hg.mozilla.org/mozilla-central/rev/161e40cc86c1 https://hg.mozilla.org/mozilla-central/rev/40aa5327f83e https://hg.mozilla.org/mozilla-central/rev/a0b4612ed03d https://hg.mozilla.org/mozilla-central/rev/2f4dc776c043

Looks like Android does not support pthread_condattr_setclock. I feel pretty good about that last try run, so I'm going to go ahead and post the patch for review.

Well, one more to check the fallback CV implementation.

And this is where the conversion starts to pay serious dividends. 1 files changed, 67 insertions(+), 131 deletions(-)

Try is green except for a missing |explicit| on the ThreadTrampoline class: turns out it counts as a single argument constructor since var-args can be empty.

Comment on attachment 8733528 [details] [diff] [review] 10_ConditionVariable__10_watchdog-v0.diff Review of attachment 8733528 [details] [diff] [review]: ----------------------------------------------------------------- Very nice! These C++ classes are so much nicer than the NSPR C interface. ::: js/src/shell/js.cpp @@ +2956,5 @@ > if (!(t_secs <= MAX_TIMEOUT_INTERVAL)) { > JS_ReportError(cx, "Excessive sleep interval"); > return false; > } > + duration = TimeDuration::FromSeconds(t_secs); So we don't have to handle the |t_secs <= 0| case explicitly anymore? @@ +2976,5 @@ > + thread = sr->watchdogThread; > + if (thread) { > + // Set the watchdog thread to nullptr to signal its death. > + sr->watchdogThread = nullptr; > + sr->watchdogWakeup.notify_all(); Unrelated nit: notifyAll? @@ +2996,5 @@ > + while (true) { > + // We signal that the watchdog should die by nulling watchdogThread. > + if (!sr->watchdogThread) { > + break; > + } Nit: no {} @@ +3008,5 @@ > + > + // Wait, waking up every 100ms to request an interrupt callback, until > + // the watchdogTimeout is expired, at which point we need to cancel the > + // (presumably deadlocked) computation. > + sr->watchdogWakeup.wait_for(lock, TimeDuration::FromMilliseconds(100)); Does this mean a timeout(0.00001) will always timeout after at least 100 ms? I was wondering about the behavior of the following script, with/without the patch: var t0 = dateNow(); timeout(0.00001, () => print(dateNow() - t0)); while (true) {} Without the patch it prints a number < 1 here on OS X. @@ +3052,5 @@ > PR_GLOBAL_THREAD, > PR_JOINABLE_THREAD, > 0); > + if (!sr->watchdogThread) > + MOZ_CRASH("failed to create watchdog thread"); Nit: if we're going to crash, we could change the return type to 'void'. Maybe use AutoEnterOOMUnsafeRegion: the fuzzers could allocate a lot of memory, spawn a worker, and hit this crash.

Comment on attachment 8733949 [details] [diff] [review] 15_Thread__00_impl-v0.diff I think js::Thread::create still needs to grow a stack size parameter - we create JS worker threads with a smaller than default stack size, and this will become even more important if we switch to a bigger default stack size on Windows. Note that _beginthreadex should use the STACK_SIZE_PARAM_IS_A_RESERVATION flag if a non-0 stack size is passed, otherwise the stack size refers to the *initially committed* stack size, rather than the address space reservation. It might be good to coordinate with jandem on this, since he's been working on stack size issues recently (e.g. bug 1257522) :)

Comment on attachment 8733525 [details] [diff] [review] 10_ConditionVariable__00_impl-v0.diff Review of attachment 8733525 [details] [diff] [review]: ----------------------------------------------------------------- I have not gone through the XP-compatible CV implementation with a fine-toothed comb. I think it's similar to the SetEvent variants described at: http://www.cs.wustl.edu/~schmidt/win32-cv-1.html but taking note of the number of sleepers may provide robustness that the variants described in said page lack. Anyway, small amounts of feedback below; I am particularly interested in responses to comments on the XP-compatible CV implementation. ::: js/src/jsapi-tests/testThreadingConditionVariable.cpp @@ +11,5 @@ > + > +struct SharedState { > + js::Mutex mutex; > + js::ConditionVariable condition; > + bool flag; Atomic<bool>, please. @@ +19,5 @@ > + > +void > +setFlag(void* arg) > +{ > + auto& shared = *reinterpret_cast<SharedState*>(arg); I approve of using C++-style casts; in this case, however, static_cast is sufficient ([expr.static.cast]p12, if you're interested). @@ +28,5 @@ > + > +BEGIN_TEST(testThreadingConditionVariable) > +{ > + SharedState shared; > + auto thread = PR_CreateThread(PR_USER_THREAD, WDYT about defining something like: struct TestState { js::Mutex mutex; js::ConditionVariable condition; bool flag; PRThread* testThread; ...constructor initializes |flag| and PRThread*... }; and then using: auto state = MakeUnique<TestState>(); in the tests? Is that over-factoring things? @@ +32,5 @@ > + auto thread = PR_CreateThread(PR_USER_THREAD, > + setFlag, > + (void *) &shared, > + PR_PRIORITY_NORMAL, > + PR_LOCAL_THREAD, Nit: I see that PR_LOCAL_THREAD is used very rarely in Gecko, though it is used a couple of times in js/. PR_GLOBAL_THREAD makes more sense and is more consistent; while I'm unsure that there's really any difference on any of our platforms, I think PR_GLOBAL_THREAD is a little clearer...? (here and elsewhere) @@ +88,5 @@ > + js::UniqueLock<js::Mutex> lock(shared.mutex); > + while (!shared.flag) { > + auto to = mozilla::TimeStamp::Now() + mozilla::TimeDuration::FromSeconds(600); > + js::CVStatus res = shared.condition.wait_until(lock, to); > + CHECK(res == js::CVStatus::NoTimeout); gtests can possibly run things in parallel, right? So we really could timeout here if we had a lot of tests to run? ::: js/src/threading/ConditionVariable.h @@ +1,5 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ I realize that I have been inattentive to this for previous patches in this bug, but some comments here would be splendid. Even something as short as: // A polyfill for std::condition_variable. would be an improvement. @@ +38,5 @@ > + > + void notify_one(); > + void notify_all(); > + > + void wait(UniqueLock<Mutex>& lock); There is an XPCOM bug recently opened about forcing mozilla::ConditionVariable users to use the moral equivalent of wait(UniqueLock&, Predicate) by not providing this method. WDYT about doing that? ::: js/src/threading/posix/ConditionVariable.cpp @@ +53,5 @@ > + result->tv_nsec -= NanoSecPerSec; > + sec += 1; > + } > + > + // Extracting the value asserts that there was no overflow. It seems a bit odd to be zealous about checks in this method (MOZ_RELEASE_ASSERT), but to be blasé about overflow possibilities inside CheckedInt: value() is not validity-checked in release mode. ::: js/src/threading/windows/ConditionVariable.cpp @@ +80,5 @@ > + sNativeImports.InitializeConditionVariable(&cv_); > + } > + > + inline void destroy() { > + // Native condition variabled don't require cleanup. Nit: "variables". @@ +98,5 @@ > + > +// Fallback condition variable support for Windows XP and Server 2003. Given the > +// difficulty of testing on these antiquated platforms and their rapidly > +// diminishing market share, this implementation trades off some fairness and > +// accuracy for implementation simplicity. I am not totally convinced that trading off fairness is a good implementation strategy; doesn't doing that lead to possible starvation? Also, what is the trading off of "accuracy" here? Accuracy of what? @@ +152,5 @@ > + MOZ_RELEASE_ASSERT(result == WAIT_OBJECT_0); > + > + // Atomically set the wakeup mode and retrieve the number of sleepers. > + MOZ_RELEASE_ASSERT((sleepersCountAndWakeupMode_ & WAKEUP_MODE_MASK) == > + WAKEUP_MODE_NONE); I would feel slightly better about this if you moved this assert after the InterlockedExchangeAdd and tested |wcwm| instead of the member variable; a little more efficient, too. Though I suppose it shouldn't really matter, since we're holding the semaphore at this point, we should be the only one with access...right? Can we document who's supposed to be holding what locks when? @@ +273,5 @@ > + > + BOOL success = ResetEvent(wakeAllEvent_); > + MOZ_RELEASE_ASSERT(success); > + > + sleepersCountAndWakeupMode_ = 0 | WAKEUP_MODE_NONE; I assume this is safe for the same reason it's safe in the WAKEUP_MODE_ONE case?

(In reply to Jan de Mooij [:jandem] from comment #140) > ::: js/src/shell/js.cpp > @@ +2956,5 @@ > > if (!(t_secs <= MAX_TIMEOUT_INTERVAL)) { > > JS_ReportError(cx, "Excessive sleep interval"); > > return false; > > } > > + duration = TimeDuration::FromSeconds(t_secs); > > So we don't have to handle the |t_secs <= 0| case explicitly anymore? The windows backend clamps to zero to both simplify the fallback and because SleepConditionVariableCS's delay parameter is unsigned. It looks like the linux backend will just overflow. Whoops, fixed. > @@ +2976,5 @@ > > + thread = sr->watchdogThread; > > + if (thread) { > > + // Set the watchdog thread to nullptr to signal its death. > > + sr->watchdogThread = nullptr; > > + sr->watchdogWakeup.notify_all(); > > Unrelated nit: notifyAll? I wanted to use the std:: names, so that at some point we can just swap out the types. > @@ +3008,5 @@ > > + > > + // Wait, waking up every 100ms to request an interrupt callback, until > > + // the watchdogTimeout is expired, at which point we need to cancel the > > + // (presumably deadlocked) computation. > > + sr->watchdogWakeup.wait_for(lock, TimeDuration::FromMilliseconds(100)); > > Does this mean a timeout(0.00001) will always timeout after at least 100 ms? > I was wondering about the behavior of the following script, with/without the > patch: > > var t0 = dateNow(); > timeout(0.00001, () => print(dateNow() - t0)); > while (true) {} > > Without the patch it prints a number < 1 here on OS X. This would depend on which thread runs first, no? I tried to not change the semantics too much, but I think the new code is more likely to hit the 100ms path. Before After Linux 100.1xx 100.1xx Windows xx.xx 100.xx Windows bounces between 0ms and 20ms, which I guess is its minimum scheduling increment. > @@ +3052,5 @@ > > PR_GLOBAL_THREAD, > > PR_JOINABLE_THREAD, > > 0); > > + if (!sr->watchdogThread) > > + MOZ_CRASH("failed to create watchdog thread"); > > Nit: if we're going to crash, we could change the return type to 'void'. > > Maybe use AutoEnterOOMUnsafeRegion: the fuzzers could allocate a lot of > memory, spawn a worker, and hit this crash. The std:: threading classes all abort in no-exceptions builds if creation fails. I'm not sure how we're going to work with this behavior. I guess in the meantime we can use AutoEnterOOMUnsafeRegion internally, but that adds quite a bit of difficulty with getting them moved into mfbt.

Replying here because I wrote the XP condition variable polyfill. > @@ +98,5 @@ > > + > > +// Fallback condition variable support for Windows XP and Server 2003. Given the > > +// difficulty of testing on these antiquated platforms and their rapidly > > +// diminishing market share, this implementation trades off some fairness and > > +// accuracy for implementation simplicity. > > I am not totally convinced that trading off fairness is a good > implementation strategy; doesn't doing that lead to possible starvation? > > Also, what is the trading off of "accuracy" here? Accuracy of what? The comment is actually not accurate; it's a leftover from an earlier patch I submitted that was (a bit) simpler, but "suffered" from spurious wakeups, and indeterminism about what threads would actually be woken up by wake_all (it was possible that one waiter would be woken up twice and another not at all). The current implementation is actually a bit slower but it is accurate and doesn't even have spurious wakeups. So a better wording would be "it trades performance for predictable behavior". I believe since XP is increasingly rare and hard to test, that's the right tradeoff nowadays. > @@ +273,5 @@ > > + > > + BOOL success = ResetEvent(wakeAllEvent_); > > + MOZ_RELEASE_ASSERT(success); > > + > > + sleepersCountAndWakeupMode_ = 0 | WAKEUP_MODE_NONE; > > I assume this is safe for the same reason it's safe in the WAKEUP_MODE_ONE > case? Only one thread of execution can reach here at the same time. So yes, same reason.

(In reply to Nathan Froyd [:froydnj] from comment #142) > I have not gone through the XP-compatible CV implementation with a > fine-toothed comb. I think it's similar to the SetEvent variants described > at: > > http://www.cs.wustl.edu/~schmidt/win32-cv-1.html In addition to my previous comment - Although both implementations use SetEvent, the control flow is quite different. The "unfair and inaccurate" comment actually refers to an implementation based on the SetEvent solution from wistl.edu. The website already discusses the merits of the SetEvent solution (see "Evaluating the SetEvent Solution"), and identifies a couple of issues. Additionally we also need to support (an equivalent to) `pthread_cond_timedwait()` which the website doesn't provide (it suggests that we could simply pass a timeout to `WaitForMultipleObjects()`, but the author clearly didn't think it through very well, because some races can happen that make the thing lock up.) - Bert

Comment on attachment 8733949 [details] [diff] [review] 15_Thread__00_impl-v0.diff Review of attachment 8733949 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for dragging my feet on this one, you shouldn't have to wait this long for a review. Mostly looks good, but I have a few things I want to dialog about. ::: js/src/threading/Thread.h @@ +41,5 @@ > + > + public: > + Id(); > + bool operator==(const Id& aOther); > + bool operator!=(const Id& aOther) { return !operator==(aOther); } My sense is that we want to |= delete| the copy constructor/assignment and |= default| the move constructor/assignment. Or wait, would we want to define the move bits ourselves to ensure that the moved-from Id doesn't actually represent a thread anymore? @@ +48,5 @@ > + inline const PlatformData* platformData() const; > + }; > + > + // Creates a thread that does not represent a thread of execution. > + Thread() {} What is this useful for, storing things in containers? A comment to that effect would be reasonable. Is it possible to get away without having this, and just have people use .emplace_back or similar? @@ +65,5 @@ > + ~Thread() { > + MOZ_RELEASE_ASSERT(!joinable()); > + } > + > + // Move the thread into the detached state. This comment should describe what "the detached state" is. Or a comment somewhere else should. @@ +68,5 @@ > + > + // Move the thread into the detached state. > + void detach(); > + > + // Block the current thread until this Thread returns from the functor it was Nit: "Thread" isn't capitalized elsewhere, so either this should be lowercase, or lots of other places should be changed. I think the former is preferable. @@ +73,5 @@ > + // created with. > + void join(); > + > + // Return true if this thread has not yet been joined or detached, or does not > + // have an associated thread of execution. Surely a Thread without an associated thread of execution isn't joinable, because we don't have anything to pass into pthread_join or equivalent? The comment reads as though a Thread without an associated thread of execution *is* joinable; I think we need a little wordsmithing here. @@ +96,5 @@ > + // Provide a process global ID to each thread. > + Id id_; > + > + // Dispatch to per-platform implementation of thread creation. > + void create(THREAD_RETURN_TYPE (THREAD_CALL_API *aMain)(void*), void* aArg); I realize having |create| return void makes error-handling much simpler, and makes constructing Thread objects much nicer...I'm just not sure it's conducive to good OOM handling. I know we punted on handling error conditions for mutexes and such, but creating a new thread takes many more resources (e.g. the thread's stack), and I think it'd be good to be a little more careful here. WDYT? ::: js/src/threading/posix/Thread.cpp @@ +136,5 @@ > + int r = pthread_set_name_np(pthread_self(), name); > + MOZ_RELEASE_ASSERT(!r); > +#elif defined(__linux__) > + int r = prctl(PR_SET_NAME, reinterpret_cast<unsigned long>(name), 0, 0, 0); > + MOZ_RELEASE_ASSERT(!r); Linux seems to support pthread_setname_np, which would be a lot nicer than using this. It seems good to support all the cases that js/src/vm/PosixNSPR.cpp:PR_SetCurrentThreadName supports, possibly with an |#else #error| to close off this |#if| chain? ::: js/src/threading/windows/Thread.cpp @@ +146,5 @@ > + > + > + > + > +#if 0 What is this #if 0 here for? It looks like this doesn't terminate until the end of the file?