801087 - Upgrade parallel workers threadpool in preparation for Rivertrail

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Attachment: Patch.

This post upgrades the |jsworkers.cpp| threadpool to support the operations necessary for Rivertrail.  I have also taken the liberty of renaming it to |jsthreadpool.cpp|, since |jsworkers.cpp| suggested a connection to web workers.

The tests are not currently working with parallel compilation enabled, so I haven't been able to test the parallel compilation components, but I believe them to be correct.

The Rivertrail-related code does pass tests, but as this is the first patch of many to land, the various functionality is not used by the rest of the spidermonkey.

Comment 1

[David Mandelin [:dmandelin]]

Comment on attachment 670938 [details] [diff] [review]
Patch.

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

It looks like the new jsthreadpool.{h,cpp} files are missing from the patch. When you repost, could you please do the file rename in one patch and then the other changes in another so it's easier to see what's changing in the file? (Maybe Splinter actually does do the smart thing in that case, but I thought it did not.)

Comment 2

[Jeff Walden [:Waldo]]

Is this thread pool stuff JS-specific?  Or is it something other code, outside the JS engine, might be able to use?  It might be good to put this in mfbt/ instead, so that Gecko can use it if it wants.  (Impossible to say without seeing the code, of course.)

Comment 3

[Niko Matsakis [:nmatsakis]]

I'll adjust the patch as requested.

Regarding the generality: this is not especially general.  I originally started off with a more general approach but ended up specializing it to the needs at hand, in order to avoid "premature abstraction".

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Rename jsworkers to jsthreadpool

I am not sure how should review now.

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Step 2 of 2.

Comment 6

[Niko Matsakis [:nmatsakis]]

OK, I replaced the patch with a more complete one and also handled the rename correctly.  I am not sure who should be reviewing this (dmandelin? sstangl?), so I left it r? without an assignee.

Comment 7

[Niko Matsakis [:nmatsakis]]

You can find an implementation of the TaskSet interface here:

http://hg.mozilla.org/users/shu_rfrn.org/iontrail/file/405eddbb7cb2/js/src/builtin/ParallelArray.cpp#l548

Comment 8

[David Mandelin [:dmandelin]]

Comment on attachment 671030 [details] [diff] [review]
Step 2 of 2.

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

I want to review this patch. But before that, I want to check in with a couple of notes:

 - Brian will be in MV next week. It would be good to have him review it as well, or at least talk about parallel compilation and how this intersects with it.
 - From glancing at the patch, I can mention a few high-level things I spotted, some of which probably imply revisions:
   - I really like that you have comments on the class members.
   - The major classes themselves, and also jsthreadpool.h as a whole, should have some comment blocks explaining the purpose and key abstractions.
   - // _______________ doesn't match our style
   - Most importantly, from a glance at jsthreadpool.h, it looks like there may not be a really clear separation between generic threading support, River Trail execution facilities, and parallel compilation facilities, although I can't be sure without more detailed examination. I would definitely want each major piece to be clear about whether it's generic or specialized, and I wouldn't want anything that's specialized to both River Trail and parallel jitting without a good excuse. 

So, given that, do you want review now, or do you want to talk design/revise/wait for Brian first?

Comment 9

[Niko Matsakis [:nmatsakis]]

I was under the impression that Brian would be gone for longer.  I'm happy to wait until next week.  

Regarding // ______, is there an intended "section marker" that does match your style?  Or just nothing?

Regarding the intermingling of concerns regarding Rivertrail and so forth, I have tried for an interface that is generic but not overly generic.  The current design (intentionally) does not include "generic" multi-threaded facilities.  I originally had a very generic design and found it was not especially readable or nice to use, so I decided to reverse course and make something narrowly tailored to the precise requirements.

However, thinking now, I imagine there is a middle ground which is not as generic as my original design but still contains a common abstraction that is used by both the parallel compilation code and the multithreaded execution code.  I can do some refactoring in that direction this week and present a modified patch once Brian is back.

Comment 10

[David Mandelin [:dmandelin]]

(In reply to Niko Matsakis from comment #9)
> I was under the impression that Brian would be gone for longer.  I'm happy
> to wait until next week.  
> 
> Regarding // ______, is there an intended "section marker" that does match
> your style?  Or just nothing?

I actually didn't know the answer to that one, but on checking I found

/*****************************************************************************/

in some of the newer files, which looks fine to me.

> Regarding the intermingling of concerns regarding Rivertrail and so forth, I
> have tried for an interface that is generic but not overly generic.  The
> current design (intentionally) does not include "generic" multi-threaded
> facilities.  I originally had a very generic design and found it was not
> especially readable or nice to use, so I decided to reverse course and make
> something narrowly tailored to the precise requirements.
> 
> However, thinking now, I imagine there is a middle ground which is not as
> generic as my original design but still contains a common abstraction that
> is used by both the parallel compilation code and the multithreaded
> execution code.  I can do some refactoring in that direction this week and
> present a modified patch once Brian is back.

That sounds about right to me. I agree that it's too early to create a completely generic multithreading facility (still learning what it should look like and don't know if it will be needed), that it's better to build "just enough" to support the current applications.

Comment 11

[David Mandelin [:dmandelin]]

I randomly happened to think about this again this morning and I realized that 'jsthreadpool.{h,cpp}' should be changed to go with the new dir/file naming convention. vm/ThreadPool.{h,cpp} makes sense to me.

Comment 12

[David Mandelin [:dmandelin]]

Comment on attachment 671030 [details] [diff] [review]
Step 2 of 2.

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

Clearing the review flag to satisfy the nag system. (Re-request if that's in error, but I believe you were going to post a new version of the patch.)

Comment 13

[Niko Matsakis [:nmatsakis]]

Attachment: Rename jsworkers to jsparcompile

Comment 14

[Niko Matsakis [:nmatsakis]]

Attachment: Generalize threadpool

Comment 15

[Niko Matsakis [:nmatsakis]]

OK, I just uploaded a revised version that separates out generic threading facilities and makes "parallel compilation" and "parallel js" clients of this API.  Unfortunately this was not completed in time for Brian Hackett---I didn't realize he'd only be here for one week, so I didn't prioritize it quite highly enough to make that deadline.

Comment 16

[Niko Matsakis [:nmatsakis]]

BTW---for me, when running jit-tests using the --ion-parallel-compilation flag, I get the precise same failures before and after applying this patch.

Comment 17

[David Mandelin [:dmandelin]]

Comment on attachment 676331 [details] [diff] [review]
Rename jsworkers to jsparcompile

This stuff should move to the vm/ directory instead. The "jsX" file naming convention is legacy. I'll try to make this clear so we don't have to cycle around too many times: I'll review the other patch now, and just talk about file names in that one, rather than explaining it here and then (likely) contradicting myself in the other review.

Comment 18

[David Mandelin [:dmandelin]]

Comment on attachment 676332 [details] [diff] [review]
Generalize threadpool

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

I wanted to finish this today but I have a hard stop shortly, so I'll put down the high-level stuff and the parts I had a chance to review, and then finish the rest later. First, module concepts and locations:

* jsmonitor.{h,cpp} -> vm/ThreadSync.{h,cpp}

I noted in that file that it's not really for monitors so it should be split out as Mutex and Condition. It doesn't really have to be its own file--if there was some other vm/ threading module those could go in there, but there isn't one so might as well create a new one.

* jsthreadpool.{h,cpp} -> vm/ThreadPool{.h,-inl.h,.cpp}

The idea of a pool of workers is a nice, simple concept.

* jstaskset.{h,cpp} -> ?

I'm still not terribly clear on what this is. From the name, I would guess that it's a set of tasks :-) but the obvious thing would be that it's a set of task objects, each of which is something that can be run. Instead it seems that it's using a callback interface. It's hard for me to be sure how the tasks are differentiated. It looks like it might be threadId but that's a bit surprising.

Anyway, if I'm on the right track about what this is for, then I'd call it something like ParallelJob. Maybe there is an argument for ParallelTaskSet, if 'task' has the right meaning in this context. Speaking of which, the names XContext are confusing to me--"Context" is too overloaded and too connected to JSContext.

Now, to boil that down to something actionable: please

 - add a comment to "ThreadContext" explaining what the concept is and what its function is in the parallel execution system
 - same for TaskSet
 - same for TaskSetSharedContext
 - then we can talk about names; feel free to propose some

* jsworkers.{h,cpp}: let's leave it out of this patch

With the patch, this file will be exclusively concerned with doing Ion compilation in parallel (yay!), so it can move to that directory as ion/IonThread.{h,cpp}. I wonder if we shouldn't just leave it alone for now, though. It doesn't quite work yet (AFAIK), and I think Brian wants to change it, so maybe you should just not bother with it for now, and just leave it as it is.

That's all I can do today. I think the threadpool stuff is probably going to be fine, but I want to check out a few things in it. I'll go over TaskSet later but it would be great if you could update it first. It may save time to go over the design in person so I know what I'm looking for and we can make some decisions together. I'm a little hard to track down randomly but feel free to schedule something in Zimbra.

::: js/src/jsmonitor.h
@@ +5,5 @@
> + * 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 jsmonitor_h___
> +#define jsmonitor_h___

Hmmm, I'm not sure about this one. It's not a monitor per se (shared data protected by a mutex over monitored code regions), and I doubt that C++ users will consistently use it to implement monitor abstractions. Also, it doesn't actually cover the existing use case in jsworkers.h, which uses two condition variables. I think it would be simpler to split this into class Mutex and class Condition, and then just use them as desired.

@@ +15,5 @@
> +#include "prcvar.h"
> +
> +namespace js {
> +
> +#ifndef JS_THREADSAFE

See my post to mozilla.dev.tech.js-engine.internals on JS_THREADSAFE. No action needed just yet for this bug, though.

::: js/src/jsthreadpool.cpp
@@ +205,5 @@
> +ThreadPool::init()
> +{
> +    // Compute desired number of workers based on env var or # of CPUs.
> +    size_t numWorkers = 0;
> +    char *pathreads = getenv("PATHREADS");

Is this for development testing purposes? An environment variable is OK for that purpose (although normal options would be better) if that's clearly indicated.

Comment 19

[Niko Matsakis [:nmatsakis]]

Attachment: Renamed and slightly revised threadpool API.

As suggested, I removed the changes to parallel compilation.  That patch can be moved to a separate bug.

Comment 20

[Niko Matsakis [:nmatsakis]]

Attachment: Renamed and slightly revised threadpool API.

Almost the same as the previous patch but I neglected to comment out the call to |adoptArenas()|! Sorry about that.

As I said before, this patch renames various things and also strips out the changes to make parallel compilation be hosted on the same thread pool.

Comment 21

[David Mandelin [:dmandelin]]

Comment on attachment 679853 [details] [diff] [review]
Renamed and slightly revised threadpool API.

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

Thanks so much for adding all the docs. The design is very comprehensible with that explanation available. This is ready to land. I have a few minor suggestions for the docs and some unimportant copyedits, but you can just fix things and land--let's not waste time on another cycle (unless you specifically want something more). I also did not try to model-check the locking protocols or anything, although they looked reasonable on basic inspection.

::: js/src/vm/forkjoin.cpp
@@ +19,5 @@
> +{
> +    ////////////////////////////////////////////////////////////////////////
> +    // Constant fields
> +
> +    JSContext *const cx_;          //< Current context

Change to //< to //. The < is not js/src style, and in a place where all the values were numeric, I started wondering whether it meant the variable value was less than the documented value.

@@ +30,5 @@
> +    // Per-thread arenas
> +    //
> +    // Each worker thread gets an arena to use when allocating.
> +
> +    Vector<gc::ArenaLists *, 16> arenaListss_;

Cute spelling.

@@ +365,5 @@
> +}
> +
> +void
> +ForkJoinShared::initiateRendezvous(ForkJoinSlice &slice) {
> +    /*

This is nice. Thank you. :-)

::: js/src/vm/forkjoin.h
@@ +17,5 @@
> + * with shared memory (as distinct from Web Workers).  The idea is
> + * that you have some (typically data-parallel) operation which you
> + * wish to execute in parallel across as many threads as you have
> + * available.  An example might be applying |map()| to a vector in
> + * parallel.  You would then extend the class |ForkJoinOp| to

The part of this paragraph after "vector in parallel..." is very dense. It is probably better to remove it, or put a code snippet, like:

  class MyForkJoinOp {
    ... define operations to be run in parallel
  };

  MyForkJoinOp op;
  ExecuteForkJoinOp(cx, op);

See js/public/HashTable.h for good examples (e.g., for removeFront).

Maybe even better than that would be to have sample code in a jsapitest or the like (someday).

@@ +143,5 @@
> +    // has been aborted and so you should bailout.  The function may
> +    // also rendesvous to perform GC or do other similar things.
> +    bool check();
> +
> +    // Returns the runtime.  Be wary, this is shared between all threads!

"Returns the runtime" is not necessary: the function is self-documenting.

@@ +146,5 @@
> +
> +    // Returns the runtime.  Be wary, this is shared between all threads!
> +    JSRuntime *runtime();
> +
> +    // Access current context using thread-local data.

This one is also self-documenting.

::: js/src/vm/threadpool.cpp
@@ +28,5 @@
> +};
> +
> +class ThreadPoolWorker : public Monitor
> +{
> +    /* Initialized at startup and never changed thereafter: */

You can remove "never changed thereafter" given that it's |const|.

@@ +125,5 @@
> +    for (;;) {
> +        while (!worklist_.empty()) {
> +            TaskExecutor *task = worklist_.popCopy();
> +            {
> +                AutoUnlockMonitor unlock(*this);

Maybe add a comment here since it's not immediately obvious why you relinquish the lock. Is it to avoid deadlock if the task tries to post another task to this worker?

::: js/src/vm/threadpool.h
@@ +36,5 @@
> +public:
> +    virtual void executeFromWorker(size_t workerId, uintptr_t stackLimit) = 0;
> +};
> +
> +/*

This is a great module comment.

@@ +60,5 @@
> + *
> + * The second way to submit a job is using |submitAll()|---in this
> + * case, the job will be executed by all worker threads.  This does
> + * not fail if there are no worker threads, it simply does nothing.
> + * Of course, each thread may have any number of preivously submitted

s/preivously/previously/

@@ +73,5 @@
> +    friend struct ThreadPoolWorker;
> +
> +    // Note:
> +    //
> +    // All fields here should only be modified during start-up or

Can put Note: All... to save some vertical space. Also, this would be slightly clearer if you defined startup (e.g., until point foo in function bar is reached).

@@ +80,5 @@
> +    JSRuntime *runtime_;
> +
> +    size_t nextId_;
> +
> +    /* Array of worker threads; lazilly spawned */

s/lazilly/lazily

@@ +91,5 @@
> +    ~ThreadPool();
> +
> +    bool init();
> +
> +    // Find out how many worker threads are in the pool.

My personal preference for this kind of comment is the imperative tense, e.g., "Return the number of worker threads in the pool". I don't think there is any consistent practice for function comment tense in js/src though.

@@ +94,5 @@
> +
> +    // Find out how many worker threads are in the pool.
> +    size_t numWorkers() { return workers_.length(); }
> +
> +    // Submits a job that will execute once by some worker.

I think you could actually leave off the comments for submitOne and submitAll here, given that you've explained them in detail above. (Alternatively, you could move those explanations down here.)

Comment 22

[Mozilla RelEng Bot]

Try run for bac8ad3c619a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bac8ad3c619a
Results (out of 272 total builds):
    success: 223
    warnings: 38
    failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bac8ad3c619a

Comment 23

[Mozilla RelEng Bot]

Try run for 624eafd36c7c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=624eafd36c7c
Results (out of 269 total builds):
    success: 222
    warnings: 37
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-624eafd36c7c

Comment 24

[Mozilla RelEng Bot]

Try run for e8e71bba0f7c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e8e71bba0f7c
Results (out of 267 total builds):
    exception: 1
    success: 219
    warnings: 38
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e8e71bba0f7c

Comment 25

[Mozilla RelEng Bot]

Try run for e8e71bba0f7c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e8e71bba0f7c
Results (out of 268 total builds):
    exception: 1
    success: 219
    warnings: 38
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e8e71bba0f7c

Comment 26

[Mozilla RelEng Bot]

Try run for 624eafd36c7c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=624eafd36c7c
Results (out of 270 total builds):
    success: 222
    warnings: 37
    failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-624eafd36c7c

Comment 27

[Mozilla RelEng Bot]

Try run for 624eafd36c7c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=624eafd36c7c
Results (out of 271 total builds):
    success: 222
    warnings: 37
    failure: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-624eafd36c7c

Comment 28

[Niko Matsakis [:nmatsakis]]

Attachment: Implement thread pool.

Includes miscellaneous minor changes in order to get a clean TRY run.

Comment 29

[Niko Matsakis [:nmatsakis]]

Try run from latest patch: https://tbpl.mozilla.org/?tree=Try&rev=1e338db823f3
(I seem to have forgotten to have it post to the bug)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a8867dd72a4

Niko, can you please make sure that your future patches have the necessary hg commit information included with them (user, commit message, etc)? It makes life easier for those checking in on your behalf. Instructions for how to configure hg are at the link below. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 31

[Niko Matsakis [:nmatsakis]]

Yes, sorry, I have done the steps on that page; I'm not sure why the info is not present in this particular patch.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

If you made the changes to your hgrc after you initially created the patch, you would have to qrefresh -u to pick up the changes.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a8867dd72a4

Comment 34

[Mozilla RelEng Bot]

Try run for 1e338db823f3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1e338db823f3
Results (out of 331 total builds):
    success: 316
    warnings: 14
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1e338db823f3