Closed Bug 956899 Opened 10 years ago Closed 8 years ago

Replace NSPR thread primitives with thin platform wrappers (take 2)

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Iteration:
50.4 - Aug 1

People

(Reporter: bertbelder, Assigned: terrence)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [revert bug 970063 once this lands])

Attachments

(29 files, 23 obsolete files)

2.93 KB, patch
terrence
: feedback+
Details | Diff | Splinter Review
2.97 KB, patch
Details | Diff | Splinter Review
1.33 KB, patch
Details | Diff | Splinter Review
60.51 KB, patch
Details | Diff | Splinter Review
5.79 KB, patch
Details | Diff | Splinter Review
13.54 KB, patch
Details | Diff | Splinter Review
2.46 KB, patch
jandem
: review+
terrence
: checkin+
Details | Diff | Splinter Review
8.31 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
5.13 KB, patch
jandem
: review+
terrence
: checkin+
Details | Diff | Splinter Review
2.01 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
4.27 KB, patch
nbp
: review+
terrence
: checkin+
Details | Diff | Splinter Review
4.74 KB, patch
h4writer
: review+
terrence
: checkin+
Details | Diff | Splinter Review
3.53 KB, patch
h4writer
: review+
terrence
: checkin+
Details | Diff | Splinter Review
14.41 KB, patch
fitzgen
: review+
terrence
: checkin+
Details | Diff | Splinter Review
6.53 KB, patch
fitzgen
: review+
terrence
: checkin+
Details | Diff | Splinter Review
2.47 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
1.88 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
3.23 KB, patch
n.nethercote
: review+
terrence
: checkin+
Details | Diff | Splinter Review
20.46 KB, patch
terrence
: review+
froydnj
: feedback+
terrence
: checkin+
Details | Diff | Splinter Review
32.89 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
10.43 KB, patch
froydnj
: review+
jbeich
: feedback+
terrence
: checkin+
Details | Diff | Splinter Review
6.38 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
2.78 KB, patch
fitzgen
: review+
terrence
: checkin+
Details | Diff | Splinter Review
5.13 KB, patch
luke
: review+
terrence
: checkin+
Details | Diff | Splinter Review
6.44 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
4.30 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
5.31 KB, patch
froydnj
: review+
terrence
: checkin+
Details | Diff | Splinter Review
9.67 KB, patch
jandem
: review+
terrence
: checkin+
Details | Diff | Splinter Review
4.63 KB, patch
jandem
: review+
terrence
: checkin+
Details | Diff | Splinter Review
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.
OS: Windows 7 → All
Hardware: x86_64 → All
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.
Attachment #8356278 - Flags: review+
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.
Attachment #8356279 - Flags: review?(terrence)
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
Attachment #8356279 - Flags: review?(terrence) → review+
(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.
Attachment #8356281 - Flags: review+
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().
Attachment #8356285 - Flags: review+
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.
Attachment #8356287 - Flags: review-
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)
Assignee: nobody → bertbelder
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [leave open]
Attachment #8356280 - Attachment is obsolete: true
Attachment #8356278 - Attachment is obsolete: true
Attachment #8356279 - Attachment is obsolete: true
Attachment #8356281 - Attachment is obsolete: true
Attachment #8356283 - Attachment is obsolete: true
Attachment #8356285 - Attachment is obsolete: true
Attachment #8356286 - Attachment is obsolete: true
Attachment #8356287 - Attachment is obsolete: true
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
Attachment #8362312 - Flags: feedback?(terrence)
Attachment #8362313 - Flags: feedback?(terrence)
Attachment #8362314 - Flags: feedback?(terrence)
Attachment #8362315 - Flags: feedback?(terrence)
Attachment #8362316 - Flags: feedback?(terrence)
Attachment #8362310 - Flags: review?(jwalden+bmo)
Attachment #8362311 - Flags: review?(jwalden+bmo)
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.
Attachment #8362312 - Flags: feedback?(terrence) → feedback+
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.
Attachment #8362310 - Flags: review?(jwalden+bmo)
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
Attachment #8362310 - Attachment is obsolete: true
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()|.
Attachment #8362311 - Flags: review?(jwalden+bmo) → feedback+
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.
Attachment #8363862 - Flags: review?(jdemooij)
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.)
Attachment #8363862 - Flags: review?(jdemooij) → review+
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.
Attachment #8362313 - Flags: feedback?(terrence) → feedback+
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.
Attachment #8362314 - Flags: feedback?(terrence) → feedback+
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!
Attachment #8362315 - Flags: feedback?(terrence) → feedback+
Attachment #8362316 - Flags: feedback?(terrence) → feedback+
See Also: → 946841
Blocks: 928222
@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).
Blocks: 720018
Blocks: ST-XPConnect
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 :)
Blocks: 970063
Whiteboard: [leave open] → [revert bug 970063 once this lands]
Ditto what Jan said above.
Flags: needinfo?(bertbelder)
Attachment #8363862 - Flags: checkin+
Attachment #8362311 - Attachment is obsolete: true
Attached patch mutex-wrapper-v1.diff (obsolete) — Splinter Review
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
Attachment #8362312 - Attachment is obsolete: true
Attachment #8457630 - Flags: review?(jwalden+bmo)
Attachment #8457630 - Flags: review?(jwalden+bmo)
Now that Steve's nspr auto-building works, the pressure to get this in is considerably lower.
Flags: needinfo?(bertbelder)
(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.
Attachment #8457630 - Flags: feedback?(jwalden+bmo)
Attachment #8457630 - Flags: feedback?(nfroyd)
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....)
Attachment #8457630 - Flags: feedback?(nfroyd) → feedback+
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.
Assignee: bertbelder → terrence
Attachment #8457630 - Attachment is obsolete: true
Attachment #8457630 - Flags: feedback?(jwalden+bmo)
Attachment #8725947 - Flags: review?(nfroyd)
Replace AutoLockForExclusiveAccess's PRLock with a Mutex.
Attachment #8725948 - Flags: review?(jdemooij)
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.
Attachment #8725952 - Flags: review?(nfroyd)
Replace the PRLock in JitSpewer with a Mutex.
Attachment #8725953 - Flags: review?(nicolas.b.pierron)
Replace the PRLock and custom guard object in TraceLogger with a Mutex and LockGuard.
Attachment #8725954 - Flags: review?(hv1989)
And for TraceLoggingGraph.
Attachment #8725955 - Flags: review?(hv1989)
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|.
Attachment #8725953 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8725948 [details] [diff] [review]
00_mutex__05_AutoLockForExclusiveAccess-v0.diff

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

Nice
Attachment #8725948 - Flags: review?(jdemooij) → review+
As discussed on IRC. Rename the rust-alike Mutex to ExclusiveData and move to threading/.
Attachment #8726335 - Flags: review?(nfitzgerald)
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(-)
Attachment #8726343 - Flags: review?(nfitzgerald)
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 ;)
Attachment #8726335 - Flags: review?(nfitzgerald) → review+
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);
Attachment #8726343 - Flags: review?(nfitzgerald) → review+
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.
Attachment #8726369 - Flags: review?(nfroyd)
Attached patch 05_UniqueLock__00_impl-v0.diff (obsolete) — Splinter Review
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.
Attachment #8726441 - Flags: review?(nfroyd)
Attachment #8725955 - Flags: review?(hv1989) → review+
Attachment #8725954 - Flags: review?(hv1989) → review+
Keywords: leave-open
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+?
Attachment #8726369 - Flags: review?(nfroyd) → review-
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.
Attachment #8725947 - Flags: review?(nfroyd) → review+
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>;

?
Attachment #8725952 - Flags: review?(nfroyd) → review+
(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?
Attachment #8726441 - Flags: review?(nfroyd) → review-
Attachment #8726335 - Flags: checkin+
I applied some review commentary in the wrong patch by mistake.
Flags: needinfo?(terrence)
(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.
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.
Attachment #8726369 - Attachment is obsolete: true
Attachment #8728492 - Flags: review?(nfroyd)
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?
Attachment #8728492 - Flags: review?(nfroyd) → review+
Attachment #8725947 - Flags: checkin+
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.
Attachment #8729078 - Flags: review?(nfroyd)
Apparently, I am the first to need to import something into SM from the mozglue directory.
Attachment #8729084 - Flags: review?(n.nethercote)
Attachment #8728492 - Flags: checkin+
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?
Attachment #8729084 - Flags: feedback+
Attachment #8729084 - Flags: review?(n.nethercote)
Hey Bert, very cool to see you contributing to the Mozilla codebase! Have fun! ;-)
Depends on: 1256089
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.
Attachment #8730306 - Flags: review?(n.nethercote)
Attachment #8729078 - Flags: review?(nfroyd) → review+
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.
Attachment #8730306 - Flags: review?(n.nethercote) → review+
Attachment #8725948 - Flags: checkin+
Attachment #8725952 - Flags: checkin+
Attachment #8725953 - Flags: checkin+
Attachment #8725954 - Flags: checkin+
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.
Attachment #8362314 - Attachment is obsolete: true
Comment on attachment 8726441 [details] [diff] [review]
05_UniqueLock__00_impl-v0.diff

We are opting out of this insanity.
Attachment #8726441 - Attachment is obsolete: true
Attachment #8726343 - Flags: checkin+
Attachment #8729084 - Attachment is obsolete: true
Depends on: 1256582
Depends on: 1256713
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.
Attachment #8733525 - Flags: review?(nfroyd)
And this is where the conversion starts to pay serious dividends.

 1 files changed, 67 insertions(+), 131 deletions(-)
Attachment #8733528 - Flags: review?(jdemooij)
Attached patch 15_Thread__00_impl-wip0.diff (obsolete) — Splinter Review
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.
Attachment #8733535 - Attachment is obsolete: true
Attachment #8733949 - Flags: review?(nfroyd)
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.
Attachment #8733528 - Flags: review?(jdemooij) → review+
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?
Attachment #8733525 - Flags: review?(nfroyd) → feedback+
(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?
Attachment #8733949 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] from comment #142)
> 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.

I disagree. The entire point of this test is that we are using |condition| to update |flag|. If we make |flag| an Atomic then the test will not fail, even if the CV is faulty.

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

Yeah, I know; not sure why I screwed this up.

> @@ +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?

I was planning to factor this after landing Thread, but I can do it before if you want.

> @@ +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)

Honestly, I just copy-pasted this from the one in (I think) HelperThreads.cpp. I'll change it to GLOBAL.

> @@ +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?

I don't know about gtests, but jsapi-tests certainly cannot. Some of the boxes we test on are quite slow, but I thought that 600 seconds was our default timeout for tests in any case? 

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

Good point! Done.

> @@ +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?

That's awesome and we should do it. Just not here. The one CV I've replaced so far cannot use that method with a naive replacement and I dare say there are others. It seems to me like something that should be done in a different bug, given that it's a major departure from the existing interface.

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

Good catch. I implemented this before switching everything from MOZ_ASSERT to MOZ_RELEASE_ASSERT and missed this case. I've added a release assertion that sec.isValid(). 

> ::: js/src/threading/windows/ConditionVariable.cpp
> @@ +80,5 @@
> > +    sNativeImports.InitializeConditionVariable(&cv_);
> > +  }
> > +
> > +  inline void destroy() {
> > +    // Native condition variabled don't require cleanup.
> 
> Nit: "variables".

Fixed.

> @@ +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?

I've updated the comment as Bert suggested.

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

Yeah, this got me on the first read too. It's not a bug because we only set it non-atomicaly to 0 | WAKEUP_MODE_NONE. You are certainly right that it's clearer to test wcwm though, so I've changed it.

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

I'll see if I can't come up with something.
 
> @@ +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?

I'll let Bert's answer stand.
I refactored the tests and applied the other feedback. I was not, however, able to find better wording than Bert's to describe the CV's invariants. I suppose we could summarize at the top, but I'm not sure if that would be doing the reader a service: it's the sort of module that really needs to be absorbed in one sitting.
Attachment #8733525 - Attachment is obsolete: true
Attachment #8743925 - Flags: review?(nfroyd)
Comment on attachment 8743925 [details] [diff] [review]
10_ConditionVariable__00_impl-v1.diff

Gah! Failed to qref. One moment.
Attachment #8743925 - Attachment is obsolete: true
Attachment #8743925 - Flags: review?(nfroyd)
Comment on attachment 8744017 [details] [diff] [review]
10_ConditionVariable__00_impl-v2.diff

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

OK, I think this is safe...or at least as safe as these sorts of things can get.  Apologies for the delay in reviewing this.  r=me with the changes below; your call on the static_assert.

::: js/src/jsapi-tests/testThreadingConditionVariable.cpp
@@ +11,5 @@
> +
> +struct TestState {
> +    js::Mutex mutex;
> +    js::ConditionVariable condition;
> +    bool flag;

You're right that non-atomicness is sufficient here; for some reason I thought it was being modified outside the lock.

::: js/src/threading/ConditionVariable.h
@@ +80,5 @@
> +
> +  PlatformData* platformData();
> +
> +#ifndef XP_WIN
> +  void* platformData_[sizeof(pthread_cond_t) / sizeof(void*)];

A thought: it might be worth:

  static_assert(sizeof(pthread_cond_t) / sizeof(void*) != 0 && sizeof(pthread_cond_t) % sizeof(void*) == 0, ...)

here, just to avoid very weird problems later on.  Same for the mutex patch earlier.  I mean, there's no way that assert *should* fail, but...

::: js/src/threading/windows/ConditionVariable.cpp
@@ +380,5 @@
> +  CRITICAL_SECTION* cs = &lock.lock.platformData()->criticalSection;
> +
> +  // Note that DWORD is unsigned, so we have to be careful to clamp at 0.
> +  double msecd = rel_time.ToMilliseconds();
> +  DWORD msec = msecd < 0.0 ? 0 : static_cast<DWORD>(msecd);

Paranoia dictates a check for a finite |msecd| value.
Attachment #8744017 - Flags: review?(nfroyd) → review+
Yup, good idea to both of those.
Depends on: 1269823
(In reply to Nathan Froyd [:froydnj] from comment #146)
> 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.

Quite understandable! This is a big and very complex patch; I wish I could land it in smaller pieces.

> ::: 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?

The spec has this to say: "thread::id shall be a trivially copyable class (Clause 9). The library may reuse the value of a thread::id of a terminated thread that can no longer be joined". I read this as saying "it's like a pthread_t or a HANDLE". If we think about what it would mean to make pthread_t non-copyable, it's pretty clear that it would make pthreads next to impossible to use. I think the spec is actually pretty much spot-on here. I'll add =default copy and move constructors, but I don't think the semantics should change.

> @@ +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?

That seems to be the intention. In particular, the following test stops working:

    mozilla::Vector<js::Thread> v;
    CHECK(v.growBy(N));
    for (auto i : mozilla::MakeRange(N))
        v[i] = js::Thread([](mozilla::Atomic<int>* countp){(*countp)++;}, &count);

That said, the very next test is identical except that it uses emplace_back. I'd be perfectly happy removing this, although I wish I had a wider view on what possible uses I'm not thinking off that might be disallowed. Maybe use with HashTables? I'll try crafting a HashTable test without the default constructor.

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

Good point! In particular, the thread stack will not be unrolled at process exit, which is pretty vital to know. I've added that, blocking information, Thread state information, and resource information to the docs. 

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

"Thread" is the name of the class we are currently implementing here; "thread" is the term of art for a process of execution with a shared address space. Suggestions welcome as to how we could make this distinction clearer.

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

Good catch! I think this should have been "or *false if it* does not...". I've split it into two sentences and clarified when we might be in a non-joinable state.

> @@ +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?

Yeah, it's super awkward how C++ doesn't really force you to use exceptions, but the stdlib makes absolutely zero concessions to those of us stuck without them. On one hand, I agree completely. On the other hand, I firmly believe that if you are creating a thread that does not belong to the pool of |numCPUs| threads you created at your process startup, you're doing it rong.

How about this: keep the empty thread state and add an |init| method that returns false if thread initialization fails. I'd actually be inclined to do this work after the fact and see how far we can get with pure RAII. If it turns out to be a catastrophe in practice we have a clear path forward.

> ::: 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?

Oh, hey, so it does! I've just copied the block from PosixNSPR.cpp over, since it's already tested.

> ::: 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?

Yeah, that's the prior implementation. I cribbed heavily, but ended up having to basically rewrite considering how different the std::thread interface is from what was there before.
This patch contains the feedback on top of 15_Thread__00_impl; I figure it will be easier to review this way, but I'd be happy to post a folded v1 if you prefer.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31f736e7f611
Attachment #8748712 - Flags: review?(nfroyd)
Comment on attachment 8748712 [details] [diff] [review]
16_Thread__00_feedback-v1.diff

f+ if you copy #include conditional from vm/PosixNSPR.cpp as well.

js/src/threading/posix/Thread.cpp:138:3: error: use of undeclared identifier
      'pthread_set_name_np'
  pthread_set_name_np(pthread_self(), name);
  ^
1 error generated.

$ ./mach jsapi-tests | grep '^TEST.*ThreadingThread'
TEST-PASS | testThreadingThreadVectorMoveConstruct | ok
TEST-PASS | testThreadingThreadId | ok
TEST-PASS | testThreadingThreadSetName | ok
TEST-PASS | testThreadingThreadDetach | ok
TEST-PASS | testThreadingThreadJoin | ok
Attachment #8748712 - Flags: feedback+
Thanks, Jan! Fixed. Sorry I missed that.
Comment on attachment 8748712 [details] [diff] [review]
16_Thread__00_feedback-v1.diff

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

This looks good; thanks for providing the interdiff.

You mentioned making a fallible init() method; for avoidance of doubt, were you planning on doing that in this patch or "after the fact", as you said in your original comment?

::: js/src/jsapi-tests/testThreadingThread.cpp
@@ -11,5 @@
>  #include "mozilla/Vector.h"
>  #include "jsapi-tests/tests.h"
>  #include "threading/Thread.h"
>  
> -BEGIN_TEST(testThreadingThreadNoThread)

These tests are removed because we removed Thread's no-arg constructor, correct?

::: js/src/threading/Thread.h
@@ +69,5 @@
>      MOZ_RELEASE_ASSERT(!joinable());
>    }
>  
> +  // Move the thread into the detached state without blocking. In the detatched
> +  // state, the thread continues to run until it exits, but cannot joined. After

Nit: "cannot be joined".

@@ +73,5 @@
> +  // state, the thread continues to run until it exits, but cannot joined. After
> +  // this method returns, this Thread no longer represents a thread of
> +  // execution. When the thread exits, its resources will be cleaned up by the
> +  // system. At process exit, if the thread is still running, the thread's TLS
> +  // storage will be destructed, but the thread stack will *not* be unrolled.

This is great, thank you!

@@ +78,4 @@
>    void detach();
>  
>    // Block the current thread until this Thread returns from the functor it was
> +  // created with. The threads resources will be cleaned up before this function

Nit: "The thread's resources..."

::: js/src/threading/posix/Thread.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/Assertions.h"
>  
> +#define _GNU_SOURCE

Nit-picky: can we get away with only defining this on Linux?  (Unless the *BSDs and OS X need this, too?)
Attachment #8748712 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #163)
> Comment on attachment 8748712 [details] [diff] [review]
> 16_Thread__00_feedback-v1.diff
> 
> Review of attachment 8748712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good; thanks for providing the interdiff.
> 
> You mentioned making a fallible init() method; for avoidance of doubt, were
> you planning on doing that in this patch or "after the fact", as you said in
> your original comment?

I still plan to do this "after the fact", having looked at a few of the thread uses in js/src/. I'm pretty sure it will be required at some point though, so I don't feel particularly strongly about that position if you want me to implement it up front. 

> ::: js/src/jsapi-tests/testThreadingThread.cpp
> @@ -11,5 @@
> >  #include "mozilla/Vector.h"
> >  #include "jsapi-tests/tests.h"
> >  #include "threading/Thread.h"
> >  
> > -BEGIN_TEST(testThreadingThreadNoThread)
> 
> These tests are removed because we removed Thread's no-arg constructor,
> correct?

Correct.

> ::: js/src/threading/Thread.h
> @@ +69,5 @@
> >      MOZ_RELEASE_ASSERT(!joinable());
> >    }
> >  
> > +  // Move the thread into the detached state without blocking. In the detatched
> > +  // state, the thread continues to run until it exits, but cannot joined. After
> 
> Nit: "cannot be joined".

Fixed.

> @@ +78,4 @@
> >    void detach();
> >  
> >    // Block the current thread until this Thread returns from the functor it was
> > +  // created with. The threads resources will be cleaned up before this function
> 
> Nit: "The thread's resources..."

Fixed.

> ::: js/src/threading/posix/Thread.cpp
> @@ +5,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #include "mozilla/Assertions.h"
> >  
> > +#define _GNU_SOURCE
> 
> Nit-picky: can we get away with only defining this on Linux?  (Unless the
> *BSDs and OS X need this, too?)

Turns out it's already defined by something above us, so we can get away with it. I tested with clang and gcc and both seemed to work locally. The BSD's actually have a different header for this, per Jan's comment above.

One last try run coming up.
We can "get away with*out* it", rather.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dba9974342da06718354291a6640d8e4b4b0c1f6
Bug 956899 - Add std::threading work-alike wrapper for posix and windows threads; r=froydnj
Attachment #8733949 - Flags: review+
Attachment #8733949 - Flags: checkin+
Attachment #8744017 - Flags: checkin+
Attachment #8748712 - Flags: checkin+
Attachment #8730306 - Flags: checkin+
The promised followup to make Thread initialization optionally fallible. I used this mechanism to implement the condition variable tests without resorting to Maybe.

Note: this still needs the ability to specify stack sizes before we can use it in actual anger. I think the way this should work is to give Thread() an optional options parameter that can specify this and anything else that comes up. Uses that need to specify options can do so by using the fallible interface.
Attachment #8749834 - Flags: review?(nfroyd)
Comment on attachment 8749836 [details] [diff] [review]
17_Thread__10_use_thread_for_ExclusiveData_tests-v0.diff

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

Thanks!
Attachment #8749836 - Flags: review?(nfitzgerald) → review+
use our std::thread polyfill in WasmSignalHandlers instead of PRThread.
Attachment #8749878 - Flags: review?(luke)
(In reply to Terrence Cole [:terrence] from comment #168)
> Note: this still needs the ability to specify stack sizes before we can use
> it in actual anger. I think the way this should work is to give Thread() an
> optional options parameter that can specify this and anything else that
> comes up.

nbp suggested on IRC making the stack size an optional template parameter. I think making it the first parameter with a default *might* work, even though though the others don't have defaults, because the others should get inferred automatically.

Of course, that's less flexible than what you were talking about, but it might be aesthetically pleasing if it works (I don't think there are any places where we don't know the stack size at compile time).
Comment on attachment 8749878 [details] [diff] [review]
17_Thread__15_use_thread_for_WasmSignalHandlers-v0.diff

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

Well that's nice.
Attachment #8749878 - Flags: review?(luke) → review+
c:\t1\hg\comm-central\mozilla\js\src\threading/Thread.h(46) : error C2610: 'js::Thread::Id::Id(js::Thread::Id &&)' : is not a special member function which can be defaulted
c:\t1\hg\comm-central\mozilla\js\src\threading/Thread.h(48) : error C2610: 'js::Thread::Id &js::Thread::Id::operator =(js::Thread::Id &&)' : is not a special member function which can be defaulted
c:/t1/hg/comm-central/mozilla/js/src/threading/windows/Thread.cpp(55) : error C2264: 'js::Thread::Id::operator =' : error in function definition or declaration; function not called
c:/t1/hg/comm-central/mozilla/js/src/threading/windows/Thread.cpp(63) : error C2264: 'js::Thread::Id::operator =' : error in function definition or declaration; function not called
c:/t1/hg/comm-central/mozilla/js/src/threading/windows/Thread.cpp(87) : error C2264: 'js::Thread::Id::operator =' : error in function definition or declaration; function not called
c:/t1/hg/comm-central/mozilla/js/src/threading/windows/Thread.cpp(96) : error C2264: 'js::Thread::Id::operator =' : error in function definition or declaration; function not called
c:/t1/hg/comm-central/mozilla/js/src/threading/windows/Thread.cpp(106) : error C2264: 'js::Thread::Id::Id' : error in function definition or declaration; function not called
Depends on: 1270714
VS 2013 does not support `default` for move constructors
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #173)
> (In reply to Terrence Cole [:terrence] from comment #168)
> > Note: this still needs the ability to specify stack sizes before we can use
> > it in actual anger. I think the way this should work is to give Thread() an
> > optional options parameter that can specify this and anything else that
> > comes up.
> 
> nbp suggested on IRC making the stack size an optional template parameter. I
> think making it the first parameter with a default *might* work, even though
> though the others don't have defaults, because the others should get
> inferred automatically.

This technique could be used, but...let's not.  An options parameter is a much cleaner approach.
(In reply to Nathan Froyd [:froydnj] from comment #177)
> This technique could be used, but...let's not.  An options parameter is a
> much cleaner approach.

What makes an options parameter somewhat awkward is that |args| is a variable template - so it isn't that clear where to *put* an options parameter (and aesthetically it's a deviation from std::thread). I don't have a strong opinion either way though.
Comment on attachment 8749834 [details] [diff] [review]
17_Thread__05_support_fallible_creation-v0.diff

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

WFM.

::: js/src/threading/Thread.h
@@ +68,5 @@
> +  // Start a thread of execution at functor |f| with parameters |args|. This
> +  // method will return false if thread creation fails. This Thread must not
> +  // already have been created.
> +  template <typename F, typename... Args>
> +  bool init(F&& f, Args&&... args) {

Please add MOZ_MUST_USE here, so people have a harder time ignoring thread initialization failure.
Attachment #8749834 - Flags: review?(nfroyd) → review+
How do I fix the errors in comment 175?
Flags: needinfo?(terrence)
(In reply to Philip Chee from comment #185)
> How do I fix the errors in comment 175?

I think you should be fine making the |= default| Id constructors conditional on MSVC 2015. I'd be happy to review any patches.
Flags: needinfo?(terrence)
(In reply to Terrence Cole [:terrence] from comment #187)
> (In reply to Philip Chee from comment #185)
> > How do I fix the errors in comment 175?
> 
> I think you should be fine making the |= default| Id constructors
> conditional on MSVC 2015. I'd be happy to review any patches.
Thanks! I didn't expect the build to work without any of the constructors. I'm currently downloading VS2015u2 but with my internet connection speeds it will probably take me several days.
> I think you should be fine making the |= default| Id constructors conditional on
> MSVC 2015. I'd be happy to review any patches.
Ah, how do I test for MSVC <= 2015
backed out for breaking 10.7 builds like https://treeherder.mozilla.org/logviewer.html#?job_id=27820847&repo=mozilla-inbound
Flags: needinfo?(terrence)
Terrence, seems the line that causing the problem is :

 16:53:46 INFO - Hit MOZ_CRASH() at /builds/slave/m-in-m64-st-an-d-0000000000000/build/src/js/src/asmjs/WasmSignalHandlers.cpp:975
Attached patch Attempt to fix VS2013 bustage (obsolete) — Splinter Review
Well this about exhausts my knowledge of C.

Unified_cpp_js_src_jsapi-tests5.obj : error LNK2019: unresolved external symbol "public: __thiscall js::Thread::Id::Id(class js::Thread::Id const &)" (??0Id@Thread@js@@QAE@ABV012@@Z) referenced in function "public: __thiscall js::Thread::~T
hread(void)" (??1Thread@js@@QAE@XZ)
js_static.lib(Thread.obj) : error LNK2001: unresolved external symbol "public: __thiscall js::Thread::Id::Id(class js::Thread::Id const &)" (??0Id@Thread@js@@QAE@ABV012@@Z)
Unified_cpp_js_src_jsapi-tests5.obj : error LNK2019: unresolved external symbol "public: class js::Thread::Id & __thiscall js::Thread::Id::operator=(class js::Thread::Id &&)" (??4Id@Thread@js@@QAEAAV012@$$QAV012@@Z) referenced in function "
public: void __thiscall js::detail::ThreadTrampoline<class <lambda_01a373175638db8ec53dae05d8bfcfd1>,class js::Thread::Id *>::callMain<0>(struct mozilla::IndexSequence<0>)" (??$callMain@$0A@@?$ThreadTrampoline@V<lambda_01a373175638db8ec53da
e05d8bfcfd1>@@PAVId@Thread@js@@@detail@js@@QAEXU?$IndexSequence@$0A@@mozilla@@@Z)
js_static.lib(Thread.obj) : error LNK2001: unresolved external symbol "public: class js::Thread::Id & __thiscall js::Thread::Id::operator=(class js::Thread::Id &&)" (??4Id@Thread@js@@QAEAAV012@$$QAV012@@Z)
js_static.lib(Thread.obj) : error LNK2019: unresolved external symbol "public: class js::Thread::Id & __thiscall js::Thread::Id::operator=(class js::Thread::Id const &)" (??4Id@Thread@js@@QAEAAV012@ABV012@@Z) referenced in function "public:
 __thiscall js::Thread::Thread(class js::Thread &&)" (??0Thread@js@@QAE@$$QAV01@@Z)
jsapi-tests.exe : fatal error LNK1120: 3 unresolved externals
Attachment #8752232 - Flags: feedback?(terrence)
Repushed the patches that aren't implicated by the failures.
(In reply to Carsten Book [:Tomcat] - PTO-back May 22th from comment #192)
> Terrence, seems the line that causing the problem is :
> 
>  16:53:46 INFO - Hit MOZ_CRASH() at
> /builds/slave/m-in-m64-st-an-d-0000000000000/build/src/js/src/asmjs/
> WasmSignalHandlers.cpp:975

Note to self: the print above this indicates that it was a MACH_RCV_INVALID_NAME error from mach_msg.
Flags: needinfo?(terrence)
Comment on attachment 8752232 [details] [diff] [review]
Attempt to fix VS2013 bustage

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

::: js/src/threading/Thread.h
@@ +45,5 @@
> +#if defined(_MSC_VER) && _MSC_VER < 1900
> +    Id(const Id&);
> +    Id(Id&&);
> +    Id& operator=(const Id&);
> +    Id& operator=(Id&&);

No, just remove these definitions; there is no non-default instantiation. You want to invert the check and just have something like:

#if (...msvc2015 or later...)
    Id(const Id&) = default;
    Id(Id&&) = default;	
    Id& operator=(const Id&) = default;
    Id& operator=(Id&&) = default;	
#endif
Attachment #8752232 - Flags: feedback?(terrence)
(In reply to Terrence Cole [:terrence] from comment #196)
> (In reply to Carsten Book [:Tomcat] - PTO-back May 22th from comment #192)
> > Terrence, seems the line that causing the problem is :
> > 
> >  16:53:46 INFO - Hit MOZ_CRASH() at
> > /builds/slave/m-in-m64-st-an-d-0000000000000/build/src/js/src/asmjs/
> > WasmSignalHandlers.cpp:975
> 
> Note to self: the print above this indicates that it was a
> MACH_RCV_INVALID_NAME error from mach_msg.

Okay, this turned out to be a pretty wild ride.

The problem is that rvalue references automatically decay to lvalue references when passed POD data. The upshot is that JSRuntime* was becoming JSRuntime*& when packed into the args tuple. In cases where the parent ran first, that stack reference would be gone and we'd read random memory as a JSRuntime*, causing the crash.

The spec for std::thread calls for DECAY_COPY of the args in the calling thread. My reading of DECAY_COPY was pretty in-depth, but I missed that particular aspect of what it was asking for. After futzing about with RemoveReference for awhile, I realized that we already have a mozilla::Decay in tree for exactly this use.

There are also a couple more things I missed in the first pass. I'll make comments inline.
Attachment #8752409 - Flags: review?(nfroyd)
Comment on attachment 8752409 [details] [diff] [review]
17_Thread__RValue_Ref_Bug-v0.diff

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

These changes are super-duper subtle. I'm pretty confident they are right, but it's also 6PM on a Friday. Please ping me so we can discuss if anything here strikes you as odd or possibly wrong.

::: js/src/jsapi-tests/testThreadingConditionVariable.cpp
@@ +25,5 @@
>  
> +    static void setFlag(TestState* state) {
> +        js::UniqueLock<js::Mutex> lock(state->mutex);
> +        state->flag = true;
> +        state->condition.notify_one();

We get lucky here and get a compile error because of the non-copyable Mutex. DECAY_COPY actually copies the storage rather than copying the reference, as was happening before, so trying to pass *this (a TestState&) attempts to copy, which fails. The solution here is to pass a pointer, which we can copy.

::: js/src/jsapi-tests/testThreadingExclusiveData.cpp
@@ +64,1 @@
>                  return;

In this case, passing a reference does *not* result in a compile error, unfortunately. Instead, because of the DECAY_COPY, each thread gets its own |counter| and we end up double-freeing counterAndBit because every thread makes the "last" count. The solution is once again to pass by pointer.

::: js/src/jsapi-tests/testThreadingThread.cpp
@@ +79,5 @@
> +    for (size_t i = 0; i < 10000; ++i) {
> +        bool b = true;
> +        js::Thread thread([](bool bb){MOZ_RELEASE_ASSERT(bb);}, b);
> +        b = false;
> +        thread.join();

This test fails without this patch. Setting |b| on the stack should not affect |bb|, but if we do a normal copy instead of a decay copy we end up copying a reference to the child thread and racing.

::: js/src/threading/Thread.h
@@ +145,5 @@
>  template <typename F, typename... Args>
>  class ThreadTrampoline
>  {
>    F f;
> +  mozilla::Tuple<typename mozilla::Decay<Args>::Type...> args;

This is the main change. Using Decay for the storage type forces a copy of the args. This is required to be able to safely move things between threads in the presence of references.

@@ +151,5 @@
>  public:
> +  template <typename G, typename... ArgsT>
> +  explicit ThreadTrampoline(G&& aG, ArgsT&&... aArgsT)
> +    : f(mozilla::Forward<F>(aG)),
> +      args(mozilla::Forward<Args>(aArgsT)...)

This is a bit subtle. Because C++, perfect forwarding does not work unless the arguments to the function are *function* templates. Just having them be the class template definitions does not work. Without perfect forwarding we cannot, for example, Move(mutex) into a background thread because C++ would try to copy when calling this constructor.

::: js/src/threading/posix/Thread.cpp
@@ +87,5 @@
>  js::Thread::create(void* (*aMain)(void*), void* aArg)
>  {
>    int r = pthread_create(&id_.platformData()->ptThread, nullptr, aMain, aArg);
> +  if (r) {
> +    id_ = Id();

pthread_create leaves the pthread_t in an undefined state if the call fails. This would mean that a subsequent call to joinable() would also be undefined, with predictably messy consequences. I did not actually hit this in practice, but seems worth fixing.
It's really good to see progress on this bug.
 
A good rule to stick to is that all by-reference arguments must be `const`.
The google c++ style guide mandates it also: https://google.github.io/styleguide/cppguide.html#Reference_Arguments
Depends on: 1272938
Comment on attachment 8752409 [details] [diff] [review]
17_Thread__RValue_Ref_Bug-v0.diff

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

These review comments are very nice, thank you for pointing them out.  The only nit that I have is that in many (all?) cases, they should be source code comments.  Maybe not for the tests, but the Thread.{h,cpp} cases would definitely help future visitors not scratch their heads.
Attachment #8752409 - Flags: review?(nfroyd) → review+
Great point! I'll transfer those over and get this landed.
Attachment #8749834 - Flags: checkin+
Attachment #8749836 - Flags: checkin+
Attachment #8729078 - Flags: checkin+
Attachment #8725955 - Flags: checkin+
As we discussed. Requesting a stack size forces use of the fallible API, which I think is a fair tradeoff.
Attachment #8754469 - Flags: review?(nfroyd)
Comment on attachment 8754469 [details] [diff] [review]
17_Thread__20_add_stack_size_option-v0.diff

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

I think this is OK.

::: js/src/threading/windows/Thread.cpp
@@ +71,5 @@
>    // created with the latter leak a small amount of memory when they use
>    // certain msvcrt functions and then exit.
> +  uintptr_t handle = _beginthreadex(nullptr, options_.stackSize(),
> +                                    aMain, aArg,
> +                                    STACK_SIZE_PARAM_IS_A_RESERVATION,

Does this work properly for a 0 options_.stackSize()?  I can't tell from the _beginthreadex docs whether this option works correctly with a 0 stack size.  I'd sleep a little better if this was:

options_.stackSize() == 0 : 0 ? STACK_SIZE_PARAM_IS_A_RESERVATION
Attachment #8754469 - Flags: review?(nfroyd) → review+
I think I convinced myself that it was okay like that, but the docs are hardly the clearest. I'll make it contingent on us setting a stack size; as you said, it will be clearer like that regardless.
Add comments to ConditionVariable and fix an edge case that I sussed out when converting more stuff over. Specifically, passing TimeDuration::Forever() on linux results in us inevitably overflowing rel_time + abs_time. We could have people call |wait| without a timeout in this case, but it seems nicer to allow at least the Forever() case to work cleanly.

Note that I still think a runtime fatal exception is the right option for when we detect overflow in the general case: this can only happen if someone is passing a wait request longer than the expected lifetime of the universe that's not "Forever".
Attachment #8756081 - Flags: review?(nfroyd)
With all of the watchdog interfaces already converted over to TimeStamp and TimeDuration, this patch becomes largely trivial.
Attachment #8733528 - Attachment is obsolete: true
Attachment #8756089 - Flags: review?(jdemooij)
Comment on attachment 8756081 [details] [diff] [review]
10_ConditionVariable__25_add_comments_and_edge_cases-v0.diff

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

r=me

::: js/src/threading/ConditionVariable.h
@@ +44,4 @@
>    void notify_all();
>  
> +  // Block the current thread of execution until this condition variable is
> +  // woken from another thread via notify_one or notify_all.

Maybe (for parallelism with Predicate-overloaded wait) say something about how this method should be called inside a loop?  That might be kind of a mouthful, though...
Attachment #8756081 - Flags: review?(nfroyd) → review+
Attachment #8752409 - Flags: checkin+
Attachment #8754469 - Flags: checkin+
Attachment #8749878 - Flags: checkin+
Attachment #8362313 - Attachment is obsolete: true
Attachment #8362315 - Attachment is obsolete: true
The js::Thread conversion is also straightforward. We can once again use Maybe to avoid relying on nullptr.
Attachment #8756490 - Flags: review?(jdemooij)
Comment on attachment 8756089 [details] [diff] [review]
10_ConditionVariable__10_watchdog-v1.diff

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

::: js/src/shell/js.cpp
@@ -311,5 @@
>      interruptFunc(rt, NullValue()),
>      lastWarningEnabled(false),
>      lastWarning(rt, NullValue()),
> -    watchdogLock(nullptr),
> -    watchdogWakeup(nullptr),

Nice how a lot of code can be removed thanks to constructors/destructors.
Attachment #8756089 - Flags: review?(jdemooij) → review+
Depends on: 1275749
Comment on attachment 8756490 [details] [diff] [review]
17_Thread__25_watchdog-v0.diff

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

::: js/src/shell/js.cpp
@@ +3101,5 @@
>  static void
>  KillWatchdog(JSRuntime* rt)
>  {
>      ShellRuntime* sr = GetShellRuntime(rt);
> +    Maybe<Thread> thread = Nothing();

I don't know if the explicit |= Nothing()| makes it (much) more readable, but either way is fine with me.
Attachment #8756490 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f02d991078d01ae91f2b6c6ed1a6520143f2cd
Bug 956899 - Add comments to ConditionVariable and handle some edge cases gracefully; r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/67d13c5fcf841e33734c0568fffa9f7686fd264f
Bug 956899 - Use Mutex and ConditionVariable to simplify shell watchdog; r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa6b27297b6aa94abb63b2430f7879f14dfa86a
Bug 956899 - Use js::Thread for JS shell watchdog thread; r=jandem
Attachment #8752232 - Attachment is obsolete: true
Relanded the patches because the issue with jsreftests persisted after the backouts.
Attachment #8756081 - Flags: checkin+
Attachment #8756089 - Flags: checkin+
Attachment #8756490 - Flags: checkin+
Depends on: 1280064
Depends on: 1280104
Depends on: 1280107
Blocks: 1263289, 1277338
Depends on: 1280089
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [revert bug 970063 once this lands] → [revert bug 970063 once this lands] [devtools-html]
Depends on: 1284198
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
As of bug 1295741, jslock.h is gone and so is all the NSPR threading in SpiderMonkey!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 1297652
Whiteboard: [revert bug 970063 once this lands] [devtools-html] → [revert bug 970063 once this lands]
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.