773491 - Replace NSPR thread primitives with thin platform wrappers

Status: RESOLVED DUPLICATE of bug 956899

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

Wrapping the sort of basic usage we care about is trivial and comes with a nice C++ification clarity boost.  Once this and 772722 land, we can kill JS_THREADSAFE and completely remove NSPR from the build.

Comment 1

[Luke Wagner [:luke]]

I don't really think I'm the right reviewer for this.  I don't ever use these primitives or know much about the platform story...

Comment 2

[Terrence Cole [:terrence]]

Attachment: v1: fix build bustage on windows and osx.

OSX does not have clock_gettime so I updated to match the recommended gettimeofday usage from their man pages. Also, SetThreadName is not a builtin on Windows: the article I read really was suggesting that we inline the given snippet. It's pretty insane, but it was in three separate MSDN articles, so everyone else is already doing it.

Comment 3

[Mike Hommey [:glandium]]

I'll get to the review later, but I have a question: why not put these in mfbt?

Comment 4

[Jeff Walden [:Waldo]]

There's already an (incompatible) mozilla::CondVar, at least, in xpcom/glue/CondVar.h or somesuch.  The idea -- and I'm not particularly happy about it, but it may be the most plausible path forward -- is to implement it here and get the JS engine off it.  Then you change the xpcom/glue/CondVar.h to be compatible with the interface here, including changing all the callers.  Then you move this implementation into mfbt.  One step at a time, and all that.

Doing things a step at a time, with a nightly or two between each step, also means potential regressions from low-level finicky changes are easier to spot.  I don't much like that it produces intermediate steps that are somewhat worse than where we started.  But it gets the job done, without a mega-sized patch to review.

You have any better ideas for how to do this?  It certainly wouldn't be hard to persuade me this moderately-awful strategy isn't the right one.  :-)

Comment 5

[Mike Hommey [:glandium]]

I wasn't aware of the existing mozilla::CondVar. Then yeah, makes sense.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 642163 [details] [diff] [review]
v1: fix build bustage on windows and osx.

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

::: js/src/platform/CondVar.cpp
@@ +81,5 @@
> +#endif
> +}
> +
> +bool
> +CondVar::timedwait(uint64_t interval_us)

I'm not convinced it's useful to have the API take intervals in microseconds when the granularity on one of the platforms is to the millisecond. This creates disparity that I'm not sure we want to deal with.

@@ +87,5 @@
> +    MOZ_ASSERT(initialized_ && mutex_);
> +#if XP_WIN
> +    return SleepConditionVariableSRW((PCONDITION_VARIABLE)&cvar_,
> +                                     (PSRWLOCK)&mutex_->mutex_,
> +                                     interval_us / MicroSecPerMilliSec, 0);

If we want microseconds anyway, then this should be fixed such that if e.g. interval_us is < 1000, it rounds to 1, not 0. (so something like (interval_us + MicroSecPerMilliSec - 1) / MicroSecPerMilliSec)
All that being said, there's a problem here because the windows API you're using only exists in Vista+, so that leaves out XP. I have absolutely no idea what XP has for that. A quick glance at the nspr code suggests nspr has its own implementation based on locks.

@@ +91,5 @@
> +                                     interval_us / MicroSecPerMilliSec, 0);
> +#else
> +    struct timespec ts;
> +#  ifdef HAVE_CLOCK_GETTIME
> +    clock_gettime(CLOCK_REALTIME, &ts);

I think it would be better to use CLOCK_MONOTONIC, in which case you'd also need a pthread_condattr_setclock in CondVar::init.

::: js/src/platform/CondVar.h
@@ +24,5 @@
> +class Mutex;
> +
> +class CondVar
> +{
> +    CVarType cvar_;

Style question: why clutter all private members with a trailing underscore?

::: js/src/platform/Mutex.cpp
@@ +18,5 @@
> +Mutex::init()
> +{
> +    MOZ_ASSERT(!initialized_);
> +#ifdef XP_WIN
> +    InitializeSRWLock((PSRWLOCK)&mutex_);

Same problem as CondVar, this is Vista+. You could use CriticalSections instead.

::: js/src/platform/Thread.cpp
@@ +14,5 @@
> +
> +namespace js {
> +
> +bool
> +Thread::run(Main main, void *data)

It may be desirable to allow to set an alternate stack size for the thread. (especially on linux where the default is 8MB. multiplication of threads can lead to address space starvation)

@@ +45,5 @@
> +    started_ = false;
> +    return true;
> +}
> +
> +void*

ThreadId

@@ +52,5 @@
> +#if XP_WIN
> +    return (void *)GetCurrentThreadId();
> +#else
> +    return (void *)pthread_self();
> +#endif

Not entirely sure this is useful in this form.

@@ +56,5 @@
> +#endif
> +}
> +
> +void
> +Thread::setName(const char *name)

Might as well allow to specify a ThreadId.

@@ +81,5 @@
> +#  if (defined(XP_MACOSX) || defined(DARWIN)) && defined(JS_CPU_X64)
> +    pthread_setname_np(name);
> +#  elif __GLIBC__ >= 2 && __GLIBC_MINOR__ >= 12
> +    pthread_setname_np(pthread_self(), name);
> +#  endif

# else
# error

Note there effectively is no implementation for mac x86.

::: js/src/platform/Thread.h
@@ +29,5 @@
> +    typedef void (*Main)(void *data);
> +    typedef void *ThreadId;
> +
> +    Thread() : thread_(), started_(false) {}
> +    bool run(Main main, void *data);

API-wise, I think static Thread *launch(...) would be nicer, but I'll leave it to Waldo or maybe someone else (bsmedberg?) to comment on that.

Comment 7

[Terrence Cole [:terrence]]

(In reply to Mike Hommey [:glandium] from comment #6)

Thank you for the review!  I am, in particular, a noob at Windows, so the detailed comments there are particularly helpful.

> ::: js/src/platform/CondVar.cpp
> @@ +81,5 @@
> > +#endif
> > +}
> > +
> > +bool
> > +CondVar::timedwait(uint64_t interval_us)
> 
> I'm not convinced it's useful to have the API take intervals in microseconds
> when the granularity on one of the platforms is to the millisecond. This
> creates disparity that I'm not sure we want to deal with.

This was mostly for convenient use with PRMJ_Now(), which returns microsecond accuracy.

> @@ +87,5 @@
> > +    MOZ_ASSERT(initialized_ && mutex_);
> > +#if XP_WIN
> > +    return SleepConditionVariableSRW((PCONDITION_VARIABLE)&cvar_,
> > +                                     (PSRWLOCK)&mutex_->mutex_,
> > +                                     interval_us / MicroSecPerMilliSec, 0);
> 
> If we want microseconds anyway, then this should be fixed such that if e.g.
> interval_us is < 1000, it rounds to 1, not 0. (so something like
> (interval_us + MicroSecPerMilliSec - 1) / MicroSecPerMilliSec)

Good catch.  Now that you mention it, it would probably be better to keep this logic wrapped in CondVar: the prior NSPR users were just dividing by _TICKS to get to the resolution they needed, so it must have had this problem already.  Even if we go with a millisecond interface, it will still be a total footgun: ((wakeup - PRMJ_Now()) / 1000) is the "obvious" thing to do, even if wrong.

> All that being said, there's a problem here because the windows API you're
> using only exists in Vista+, so that leaves out XP. I have absolutely no
> idea what XP has for that. A quick glance at the nspr code suggests nspr has
> its own implementation based on locks.

Huh, I thought our buildbots included XP.  I'll take a look at NSPR and see what's needed here.
 
> ::: js/src/platform/CondVar.h
> @@ +24,5 @@
> > +class Mutex;
> > +
> > +class CondVar
> > +{
> > +    CVarType cvar_;
> 
> Style question: why clutter all private members with a trailing underscore?

I personally prefer an m prefix, but I'm basically the only one who works on SpiderMonkey.  Feel free to take it up with the rest of the team, maybe you'll get farther than I did :-).
 
> ::: js/src/platform/Mutex.cpp
> @@ +18,5 @@
> > +Mutex::init()
> > +{
> > +    MOZ_ASSERT(!initialized_);
> > +#ifdef XP_WIN
> > +    InitializeSRWLock((PSRWLOCK)&mutex_);
> 
> Same problem as CondVar, this is Vista+. You could use CriticalSections
> instead.

I used SRWLOCK because it is a struct{void*}, which is easy to get without #including windows.h in a header.  CRITICAL_SECTION will pretty much require us to #include windows.h everywhere, including in exported JSAPI.  Uhg.  I have a few "clever" ideas that could get around this if it comes to that, but none of it is pretty.

> ::: js/src/platform/Thread.cpp
> @@ +14,5 @@
> > +
> > +namespace js {
> > +
> > +bool
> > +Thread::run(Main main, void *data)
> 
> It may be desirable to allow to set an alternate stack size for the thread.
> (especially on linux where the default is 8MB. multiplication of threads can
> lead to address space starvation)

Good point.  We only have one thread is SM, but it doesn't need 8MB of stack either.
 
> @@ +52,5 @@
> > +#if XP_WIN
> > +    return (void *)GetCurrentThreadId();
> > +#else
> > +    return (void *)pthread_self();
> > +#endif
> 
> Not entirely sure this is useful in this form.

What exactly do you mean?
 
> @@ +56,5 @@
> > +#endif
> > +}
> > +
> > +void
> > +Thread::setName(const char *name)
> 
> Might as well allow to specify a ThreadId.

On OSX, you can only set the name on the current thread, so providing a ThreadId, or making it instance-specific would be deceptive.
 
> @@ +81,5 @@
> > +#  if (defined(XP_MACOSX) || defined(DARWIN)) && defined(JS_CPU_X64)
> > +    pthread_setname_np(name);
> > +#  elif __GLIBC__ >= 2 && __GLIBC_MINOR__ >= 12
> > +    pthread_setname_np(pthread_self(), name);
> > +#  endif
> 
> # else
> # error
> 
> Note there effectively is no implementation for mac x86.

Correct.  pthread_setname(_np) is simply not implemented in the older versions of the OS.  Since the older versions only support x86, and (afaik) Apple switched to x64 hardware at the same time as they introduced x64 support, I used x64 as an easy version filter.  This will miss some upgraded machines, but I did not want to add more autoconfigury for a method that is basically just cosmetic.
 
> ::: js/src/platform/Thread.h
> @@ +29,5 @@
> > +    typedef void (*Main)(void *data);
> > +    typedef void *ThreadId;
> > +
> > +    Thread() : thread_(), started_(false) {}
> > +    bool run(Main main, void *data);
> 
> API-wise, I think static Thread *launch(...) would be nicer, but I'll leave
> it to Waldo or maybe someone else (bsmedberg?) to comment on that.

I went with pthreads naming as it is the standard, but what I should have done, in hindsight, was to just make the interface identical to the one in xpcom.  I'll do that for the next round.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Terrence Cole [:terrence] from comment #7)
> Huh, I thought our buildbots included XP.  I'll take a look at NSPR and see
> what's needed here.

We don't build on XP but we run tests on XP. However, on try, except if you use "-p all" XP tests won't run. And if you don't run tests on XP, you won't notice that the functions don't exist there, since building will just work.

> > Style question: why clutter all private members with a trailing underscore?
> 
> I personally prefer an m prefix, but I'm basically the only one who works on
> SpiderMonkey.  Feel free to take it up with the rest of the team, maybe
> you'll get farther than I did :-).

If that's the spidermonkey style, then so be it. I just find that ugly.

> > @@ +52,5 @@
> > > +#if XP_WIN
> > > +    return (void *)GetCurrentThreadId();
> > > +#else
> > > +    return (void *)pthread_self();
> > > +#endif
> > 
> > Not entirely sure this is useful in this form.
> 
> What exactly do you mean?

I mean that I don't see where you'd be using a Thread::current that returns the native type. Especially when no function in the API takes this type. And as a matter of fact, you never use the function.
  
> On OSX, you can only set the name on the current thread, so providing a
> ThreadId, or making it instance-specific would be deceptive.

fair enough.
  
> > @@ +81,5 @@
> > > +#  if (defined(XP_MACOSX) || defined(DARWIN)) && defined(JS_CPU_X64)
> > > +    pthread_setname_np(name);
> > > +#  elif __GLIBC__ >= 2 && __GLIBC_MINOR__ >= 12
> > > +    pthread_setname_np(pthread_self(), name);
> > > +#  endif
> > 
> > # else
> > # error
> > 
> > Note there effectively is no implementation for mac x86.
> 
> Correct.  pthread_setname(_np) is simply not implemented in the older
> versions of the OS.  Since the older versions only support x86, and (afaik)
> Apple switched to x64 hardware at the same time as they introduced x64
> support, I used x64 as an easy version filter.  This will miss some upgraded
> machines, but I did not want to add more autoconfigury for a method that is
> basically just cosmetic.

x64 hardware may run the x86 part of our universal builds when using 32-bits plugins (and there are). Also, x86 hardware can run newer versions of osx. Isn't there an equivalent to __GLIBC__ in osx?

Comment 9

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #8)
> x64 hardware may run the x86 part of our universal builds when using 32-bits
> plugins (and there are). Also, x86 hardware can run newer versions of osx.
> Isn't there an equivalent to __GLIBC__ in osx?

Actually, the best thing to do is this:
extern int pthread_setname_np(const char*) __attribute__((weak_import));

void func() {
  if (pthread_setname_np)
    pthread_setname_np();
}

Comment 10

[Terrence Cole [:terrence]]

(In reply to Mike Hommey [:glandium] from comment #9)
> Actually, the best thing to do is this:
> extern int pthread_setname_np(const char*) __attribute__((weak_import));
> 
> void func() {
>   if (pthread_setname_np)
>     pthread_setname_np();
> }

That is *horrifying*...  I love it!

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 642163 [details] [diff] [review]
v1: fix build bustage on windows and osx.

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

I should not be awake at all right now, but I have to spew out what I had.  Apologies if it's incoherent in parts.

Regarding style/syntax: since the plan is to move this into mfbt, I think we should just use mfbt/STYLE rules from the get-go, to reduce the need for style munging and blame-fiddling later.  And use C++-style casts!

I kind of prefer run to launch for thread APIs because it's what I've seen more, and it's what we already do.

Probably timedwait or whatever should take the more-precise units, and its documentation should say it attempts to be as precise as possible but can't always be.  But I'm not sure how important extra precision is, or when, so more knowledgeable heads should speak to the point.  Either way you should definitely include the units in the name -- timedWaitMsec or something -- so it's clear to callers, tho.  (And camelCaps for the name, of course.)

::: js/src/platform/CondVar.h
@@ +4,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/. */
> +
> +#include "mozilla/StandardInteger.h"

The include-guard should encompass the whole file -- this #include should be inside it.  (Applies to other files too.)

@@ +39,5 @@
> +
> +    bool broadcast();
> +    bool signal();
> +    bool wait();
> +    bool timedwait(uint64_t interval_us);

This entire file needs documentation -- both at a high level, and at the level of the individual methods making up the interface.  See existing mfbt/*.h files for examples.  (We want the headers to be self-documenting, so that MDN docs can mostly just be pointers to the relevant headers.)

@@ +43,5 @@
> +    bool timedwait(uint64_t interval_us);
> +
> +    const static uint64_t MicroSecPerMilliSec = 1000UL;
> +    const static uint64_t MicroSecPerSec = 1000000UL;
> +    const static uint64_t NanoSecPerMicroSec = 1000UL;

const static uint64_t mixes metaphors.  First a type modifier, then a storage modifier, then a type.  Make that |static const uint64_t| please.

::: js/src/platform/Mutex.h
@@ +65,5 @@
> +    Mutex *mutex_;
> +    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
> +
> +  public:
> +    AutoUnlock(Mutex *mutex MOZ_GUARD_OBJECT_NOTIFIER_PARAM)

Put the guard param on a separate line, so that gunk is visually separated from the actual parameters.

::: js/src/platform/Thread.cpp
@@ +14,5 @@
> +
> +namespace js {
> +
> +bool
> +Thread::run(Main main, void *data)

This is desirable, as I think media code sets different stack sizes so that a wall of video elements doesn't kill you.

Comment 12

[Mike Hommey [:glandium]]

What happened to this bug?

Comment 13

[Terrence Cole [:terrence]]

(In reply to Mike Hommey [:glandium] from comment #12)
> What happened to this bug?

There is no CondVar implementation on WinXP, so thin wrappers aren't really an option on windows. Since the bug is not trivial and we already have a thick wrapper for CondVars on windows -- NSPR -- I decided it probably wasn't worth the effort. There are a number of slimmer thick wrappers available, but it would not really be worth the time and effort.

I left the bug open because at some point WinXP will be an ancient memory and implementing this will be trivial.

Comment 14

[Florian Bender]

I guess this is finished in Bug 956899 now.