1312085 - drop support for XP in JS's platform-independent locks and condvars

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

js::ConditionVariable and js::Mutex--especially js::Mutex--are complicated by their support for Windows XP.  Once we no longer support Windows XP, we can delete that support code, massively simplifying the classes.

(It is an open question whether we want to switch to std:: equivalents at that point; I think given things like possible support for deadlock detection, we still want to keep some sort of wrapper.)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: remove the XP fallback for JS condition variables

We no longer need the complicated XP fallback.  Since we're not yet
compiling with the proper WINVER, however, we need to retain the dynamic
loading of the condition variable APIs.

I'm ambivalent about whether we need to have the MOZ_RELEASE_ASSERTs.  A try
run certainly shows that we don't hit that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4c92a5006c25cd5a3b707312b004f2e5cec9c53

However, a little paranoia here does not seem out of place.  WDYT?

Feel free to pass the review off to someone else.  I would have thrown this at
Terrence, but as he is no longer here, I'm not sure who would review...

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nathan Froyd [:froydnj] from comment #0)
> js::ConditionVariable and js::Mutex--especially js::Mutex--are complicated
> by their support for Windows XP.  Once we no longer support Windows XP, we
> can delete that support code, massively simplifying the classes.

\o/

> (It is an open question whether we want to switch to std:: equivalents at
> that point; I think given things like possible support for deadlock
> detection, we still want to keep some sort of wrapper.)

The lock ordering assertions that jonco added are super valuable, as is our ability to set PTHREAD_MUTEX_ERRORCHECK in DEBUG builds. With the std locks, we could reimplement the first on top of them, but I don't think there is an equivalent to the second, is there?

http://searchfox.org/mozilla-central/rev/a712d69adb9b2588f88aff678216b2be94d3719c/js/src/threading/Mutex.h#69
http://searchfox.org/mozilla-central/rev/a712d69adb9b2588f88aff678216b2be94d3719c/js/src/threading/posix/MutexImpl.cpp#41

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #2)
> (In reply to Nathan Froyd [:froydnj] from comment #0)
> > (It is an open question whether we want to switch to std:: equivalents at
> > that point; I think given things like possible support for deadlock
> > detection, we still want to keep some sort of wrapper.)
> 
> The lock ordering assertions that jonco added are super valuable, as is our
> ability to set PTHREAD_MUTEX_ERRORCHECK in DEBUG builds. With the std locks,
> we could reimplement the first on top of them, but I don't think there is an
> equivalent to the second, is there?

libstdc++ doesn't have anything like the second one, AFAICT--that would be really useful!  It would be doable to add in a debug mode, but it'd be a while before Gecko could make use of that.  I haven't looked at libc++ or MSVC.

Gecko's Mutex class comes with lock ordering assertions, or deadlock detection, which I think is equivalent?  Maybe jonco's stuff is simpler than Gecko's deadlock detector, though..

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sorry for not getting to your review, I've been on vacation and dropped that last comment when I had a moment. I meant to also review, but didn't get a chance and didn't leave a comment either. In general, I think myself and jonco are good reviewers js/src/threading. Lots more js folks are very qualified, but I do not think anyone else has touched this code yet.

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Gecko's Mutex class comes with lock ordering assertions, or deadlock
> detection, which I think is equivalent?  Maybe jonco's stuff is simpler than
> Gecko's deadlock detector, though..

As I understand it, Gecko's mutex tries to determine what the ordering is at runtime by building a graph, and asserts if it observes any cycles. This is pretty neat (and I bet it was fun to write).

However, the orderings that jonco added mean that we don't need to build a graph to determine the ordering: we already explicitly stated our orderings and can assert that they are correct in practice directly. Additionally, it also serves as documentation for users about which locks they can take in which contexts. In my opinion, it is worth maintaining this scheme.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8826006 [details] [diff] [review]
remove the XP fallback for JS condition variables

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

\o/

Thanks, Nathan!

Comment 6

[Till Schneidereit [:till]]

I don't really know how ours and the std:: locking stuff compares to OS-provided facilities (though I would assume the std:: stuff just uses that?), WebKit got excellent results by using their own tailor-made WTF::Lock and WTF::Condition: https://webkit.org/blog/6161/locking-in-webkit/

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Till Schneidereit [till] from comment #6)
> I don't really know how ours and the std:: locking stuff compares to
> OS-provided facilities (though I would assume the std:: stuff just uses
> that?), WebKit got excellent results by using their own tailor-made
> WTF::Lock and WTF::Condition: https://webkit.org/blog/6161/locking-in-webkit/

I think that some folks were talking about importing that code into MFBT... That would be interesting. Maybe Nathan knows more about where that discussion went?

The std:: stuff uses the OS-provided facilities under the hood, yes.

I think what makes our stuff stand out is our dedication to DEBUG assertions, which is very valuable to us, but also largely specific to our uses (the ordering at least) but also can be rebased on top of something like WTF::Lock.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #7)
> (In reply to Till Schneidereit [till] from comment #6)
> > I don't really know how ours and the std:: locking stuff compares to
> > OS-provided facilities (though I would assume the std:: stuff just uses
> > that?), WebKit got excellent results by using their own tailor-made
> > WTF::Lock and WTF::Condition: https://webkit.org/blog/6161/locking-in-webkit/
> 
> I think that some folks were talking about importing that code into MFBT...
> That would be interesting. Maybe Nathan knows more about where that
> discussion went?

I would be in favor of using something like WTF::ParkingLog (perhaps importing the Rust parking_lot crate and using that).  However, it's worth noting that all tests in that blog post, AFAICT, were done on Mac, and we have evidence from other bugs (bug 1137963 and bug 1195767 are the ones that come to mind) that Mac pthread locks are...not the most optimized implementation of locks.  So while I am willing to believe that parking_lot or similar can provide speedups, I am also skeptical that the results the WebKit team saw are transferable to other platforms.

I have a Q1 goal to, in no particular order:

* this bug (along with a possible followup to make things use SRW locks)
* bug 1312086 (move JS lock stuff to mozglue)
* bug 1331205 (change ipc's locks to use better Windows primitives)
* make XPCOM's locks use the new mozglue locks
* make ipc code use XPCOM's locks

so everything in platform is using more-or-less the same set of optimized, OS-specific locks with no cruft (i.e. NSPR locking).  Then we can see about doing performance testing and deciding whether parking_lot is worthwhile for some or all of our platforms.

What I don't know is whether it's worth it for the mozglue locks to be wrappers around the platform-specific functionality (pthreads etc.) or whether they should just be wrappers around std:: bits.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> I would be in favor of using something like WTF::ParkingLog (perhaps
> importing the Rust parking_lot crate and using that).  However, it's worth
> noting that all tests in that blog post, AFAICT, were done on Mac, and we
> have evidence from other bugs (bug 1137963 and bug 1195767 are the ones that
> come to mind) that Mac pthread locks are...not the most optimized
> implementation of locks.  So while I am willing to believe that parking_lot
> or similar can provide speedups, I am also skeptical that the results the
> WebKit team saw are transferable to other platforms.

One thing that is an aside to time-performance is that pthreads have to be ABI compatible, and therefore have to be rather large for historical reasons. A fresh start like WTF::Lock can avoid that and means we could use more fine grained locking without introducing memory bloat.

But this is getting a bit into the weeds.

> so everything in platform is using more-or-less the same set of optimized,
> OS-specific locks with no cruft (i.e. NSPR locking).  Then we can see about
> doing performance testing and deciding whether parking_lot is worthwhile for
> some or all of our platforms.

Makes sense.

> What I don't know is whether it's worth it for the mozglue locks to be
> wrappers around the platform-specific functionality (pthreads etc.) or
> whether they should just be wrappers around std:: bits.

My gut feeling is that it makes sense to leave std:: bits out so we can keep the PTHREAD_MUTEX_ERRORCHECK stuff in DEBUG, but I trust your judgement ;)

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #9)
> (In reply to Nathan Froyd [:froydnj] from comment #8)
> > I would be in favor of using something like WTF::ParkingLog (perhaps
> > importing the Rust parking_lot crate and using that).  However, it's worth
> > noting that all tests in that blog post, AFAICT, were done on Mac, and we
> > have evidence from other bugs (bug 1137963 and bug 1195767 are the ones that
> > come to mind) that Mac pthread locks are...not the most optimized
> > implementation of locks.  So while I am willing to believe that parking_lot
> > or similar can provide speedups, I am also skeptical that the results the
> > WebKit team saw are transferable to other platforms.
> 
> One thing that is an aside to time-performance is that pthreads have to be
> ABI compatible, and therefore have to be rather large for historical
> reasons. A fresh start like WTF::Lock can avoid that and means we could use
> more fine grained locking without introducing memory bloat.

Having locks be consistent sizes on all platforms vs. the situation today (4 words on Windows, soon to be 1, 5 words on Linux, 8 words on Mac) would definitely be helpful.  But are we really interested in finer-grained locking beyond "that'd be splendid"?  Would finer-grained locking improve the JS engine in a tangible way?  My sense is that it wouldn't be that interesting for Gecko, but the JS engine has different constraints.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I'm certainly talking about this from a "that'd be splendid" sense, but bhacket is doing some stuff you may find relevant and interesting: https://bugzilla.mozilla.org/show_bug.cgi?id=1323066

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #11)
> bhacket is doing some stuff you may find relevant and interesting

Is there ever a point in time where this is not a true statement.

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 1 - remove the XP fallback for JS condition variables

This is really the same patch, but now we compile with a proper WINVER, so we
can delete all the dynamic library function loading bits. \o/

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: part 2 - remove XP conditionalization for JS mutexes

Now that we don't support XP, we can use InitializeCriticalSectionEx
explicitly, rather than calling it through a function pointer.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8831300 [details] [diff] [review]
part 1 - remove the XP fallback for JS condition variables

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

Wow! \o/

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8831301 [details] [diff] [review]
part 2 - remove XP conditionalization for JS mutexes

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

Great!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12896715acd1
part 1 - remove the XP fallback for JS condition variables; r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/2977fc85181e
part 2 - remove XP conditionalization for JS mutexes; r=fitzgen

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/12896715acd1
https://hg.mozilla.org/mozilla-central/rev/2977fc85181e

Comment 19

[Julien Cristau [:jcristau] (back April 22)]

We need xp support in 52 for ESR → wontfix.