825714 - Refactor GC/Ion fields for parallel execution

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

This is the first of several upcoming patches incorporating the work from the Parallel JS branch.

The primary purpose of this patch is to isolate the fields needed to allocate memory into a new data structure, the Allocator.  There is a primary Allocator inlined into each Compartment, but threaded execution can allocate a separate Allocator per thread.

The patch includes the following semi-independent but entangled changes:

- A new routine JSObject::parExtendDenseArray() that permits parallel code to allocate a dense array on its own Allocator.

- Create an allocation path in the GC for loading a fresh arena as needed (ArenaLists::parallelAllocate()).

- Ensure that if GC is triggered during parallel execution, parallel execution is aborted and the request is deferred until parallel execution terminates.

- Updates to the ForkJoin/ThreadPool so that they create their own Allocators as well as other misc API changes.

- Moves some of the ion-related fields (e.g., |ionTop|) into perThreadData.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to implement described changes.

Try Server URL: https://tbpl.mozilla.org/?tree=Try&rev=ad76d6432c60

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to implement described changes.

Try run: http://tbpl.mozilla.org/?tree=Try&rev=1b37d356fd32

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to implement described changes.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9798e13079e5

Comment 4

[Mozilla RelEng Bot]

Try run for 9798e13079e5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9798e13079e5
Results (out of 281 total builds):
    exception: 8
    success: 194
    warnings: 72
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9798e13079e5

Comment 5

[Niko Matsakis [:nmatsakis]]

Try run with fix for arm errors (will push final patch soon): https://tbpl.mozilla.org/?tree=Try&rev=05f9f52a4f63

Comment 6

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to implement described changes.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=66882f4d77f7

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 698040 [details] [diff] [review]
Patch to implement described changes.

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

As a first pass, I don't like how you have to include ForkJoin-inl.h from all sorts of weird places. It seems like the two functions there could just as easily go in ForkJoin.h. (We usually only put files in -inl.h files if we have to because of a dependency.) Then you could include ForkJoin.h from jsinterpinlines.h and remove all the other includes.

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to implement described changes.

This version inlines ForkJoin-inl.h into ForkJoin.h

Try run: http://tbpl.mozilla.org/?tree=Try&rev=cccf90fb75a8

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 698833 [details] [diff] [review]
Patch to implement described changes.

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

::: js/src/ion/IonFrames-inl.h
@@ +85,4 @@
>  GetTopIonJSScript(JSContext *cx, const SafepointIndex **safepointIndexOut, void **returnAddrOut)
>  {
>      AutoAssertNoGC nogc;
> +    IonFrameIterator iter(cx->runtime->mainThread.ionTop);

Could this be changed to an accessor like cx->ionTop() or cx->runtime->ionTop() (here and everywhere else)? These long chains can get annoying to read and type.

Comment 10

[Niko Matsakis [:nmatsakis]]

Yes, I can add an accessor.  One option is: "cx->mainThread().ionTop", which is not so far off from "cx->runtime->ionTop".  The advantage of that would be that we don't need to add a new accessor each time a new field is added to mainThread.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Sounds good.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 698833 [details] [diff] [review]
Patch to implement described changes.

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

As an overall comment, then name ForkJoinSlice isn't very good when used in static functions like ForkJoinSlice::Initialize and InParallelSection. Could you make a new class for this code, with a name like ForkJoin or Rivertrail or something?

Also, you should assert !InParallelSection from NewGCThing and TryNewGCThing.

::: js/src/ion/Ion.cpp
@@ +23,3 @@
>  #include "IonCompartment.h"
>  #include "CodeGenerator.h"
> +#include "jsworkers.h"

The order of includes is pretty messed up here (even before your patch). The order should be:

1. first, all the jsxyz.h files alphabetically
2. then all the RangeAnalysis.h files (i.e., ones in the ion directory) alphabetically
3. then files in other dirs like vm/Xyz.h alphabetically
4. then all the inlines files, in pretty much the same order as above

::: js/src/jsarray.cpp
@@ +2179,4 @@
>                  result = vec.begin() + n;
>              }
>          } else {
> +            // note: currently, array.sort() cannot be used from parallel code

We use /* */-style comments in this file. Also, there's no need for "note:".

::: js/src/jsarrayinlines.h
@@ +17,5 @@
>      MarkTypeObjectFlags(cx, this, js::types::OBJECT_FLAG_NON_PACKED_ARRAY);
>  }
>  
> +inline JSObject::EnsureDenseResult
> +JSObject::parExtendDenseArray(js::Allocator *alloc, js::Value *v, uint32_t extra)

Brian recently changed how this works in bug 827490, so I'm afraid you're going to have to do some rebasing here.

::: js/src/jscntxt.h
@@ +1081,5 @@
>       * Call the system malloc while checking for GC memory pressure and
>       * reporting OOM error when cx is not null. We will not GC from here.
>       */
> +    void* malloc_(size_t bytes, JSCompartment *comp = NULL, JSContext *cx = NULL) {
> +        updateMallocCounter(comp, bytes);

Can you assert before this that comp is NULL if and only if cx is NULL? I want to make sure we don't fail to update the compartment's counter in cases where we would have updated it before.

::: js/src/jscompartment.h
@@ +130,5 @@
> + * |Allocator*| rather than a |JSContext*|.
> + */
> +class Allocator
> +{
> +    JSCompartment         *const compartment;

This should be indented as far over as the other fields here.

@@ +138,5 @@
> +     * compartment's GC things. This counter should be used in preference to
> +     * gcMallocBytes. These counters affect collection in the same way as
> +     * gcBytes and gcTriggerBytes.
> +     */
> +    size_t                       gcMallocAndFreeBytes;

I'd rather you not move this field or any of the related stuff. It's not actually used and I'm planning to remove it.

@@ +140,5 @@
> +     * gcBytes and gcTriggerBytes.
> +     */
> +    size_t                       gcMallocAndFreeBytes;
> +
> +public:

This should be indented two spaces.

@@ +214,5 @@
> +
> +    /*
> +     * Moves all data from the allocator |workerAllocator|, which was
> +     * in use by a parallel worker, into the compartment's main
> +     * allocator.  This is used at the end of a parallel section. */

The */ should be on its own line.

@@ +500,5 @@
>      void setGCMaxMallocBytes(size_t value);
>      void updateMallocCounter(size_t nbytes) {
> +        /* Note: this code may be run from worker threads.
> +
> +           We tolerate any thread races when updating gcMallocBytes. */

This should be formatted as follows:

/*
 * This code may be run from worker threads. We tolerate races when updating
 * gcMallocBytes.
 */

::: js/src/jscompartmentinlines.h
@@ +27,5 @@
>      cx_->leaveCompartment(origin_);
>  }
>  
> +inline void *
> +js::Allocator::malloc_(size_t bytes) {

In this function and the ones that follow, the brace should go on its own line.

@@ +37,5 @@
> +    return compartment->rt->calloc_(bytes, compartment);
> +}
> +
> +inline void *
> +js::Allocator::realloc_(void* p, size_t bytes) {

It should be |void *p|, not |void* p|.

::: js/src/jsfriendapi.h
@@ +561,4 @@
>  inline uintptr_t
>  GetNativeStackLimit(const JSRuntime *rt)
>  {
> +    return PerThreadDataFriendFields::getMainThread(rt)->nativeStackLimit;

I just noticed that we no longer need GetNativeStackLimit to be in jsfriendapi any more. The only user is in Marking.cpp, and it can access the nativeStackLimit field directly.

I think that means you can revert the change to the static assert in jsfriendapi.cpp and remove getMainThread from jspubtd.h.

@@ +741,4 @@
>      D(DEBUG_GC)                                 \
>      D(DEBUG_MODE_GC)                            \
>      D(TRANSPLANT)                               \
> +    D(START_PARALLEL_BLOCK)                     \

This is going to mess up telemetry. Can you just use API instead?

::: js/src/jsgc.cpp
@@ +1105,5 @@
>  
> +void *
> +ArenaLists::parallelAllocate(JSCompartment *comp, AllocKind thingKind, size_t thingSize)
> +{
> +    void *t = allocateFromFreeList(thingKind, thingSize);

Please add a comment here like "During parallel Rivertrail sections, no GC is permitted. If no existing arena can satisfy the allocation, then a new one is allocated. If that fails, then we OOM."

@@ +1121,5 @@
> +    // This function can be called from parallel threads all of which
> +    // are associated with the same compartment. In that case, each
> +    // thread will have a distinct ArenaLists.  Therefore, whenever we
> +    // fall through to PickChunk() we must be sure that we are holding
> +    // a lock.

Please use /* */-style comments. Also, change "Threading Note:" to "Rivertrail Note:" so people know this doesn't happen normally.

@@ +1398,5 @@
> +    // Note: we do not attempt the last ditch GC in a parallel
> +    // section.  Of course we could modify the `runGC` flag below but
> +    // since that path is quite hot is was deemed better to offload
> +    // the access to thread-local data into this function.
> +    if (ForkJoinSlice::InParallelSection())

I don't think RunLastDitchGC will ever be called from a parallel section, so please change this to an assertion.

@@ +1862,4 @@
>  void
>  js::TriggerGC(JSRuntime *rt, gcreason::Reason reason)
>  {
> +    // Wait till end of parallel section to trigger GC.

/* */-style comment

@@ +1863,5 @@
>  js::TriggerGC(JSRuntime *rt, gcreason::Reason reason)
>  {
> +    // Wait till end of parallel section to trigger GC.
> +    ForkJoinSlice *slice = ForkJoinSlice::Current();
> +    if (slice != NULL) {

Change to:
  if (ForkJoinSlice *slice = ForkJoinSlice::Current()) {

@@ +1881,4 @@
>  void
>  js::TriggerCompartmentGC(JSCompartment *comp, gcreason::Reason reason)
>  {
> +    // Wait till end of parallel section to trigger GC.

/* */

@@ +1882,5 @@
>  js::TriggerCompartmentGC(JSCompartment *comp, gcreason::Reason reason)
>  {
> +    // Wait till end of parallel section to trigger GC.
> +    ForkJoinSlice *slice = ForkJoinSlice::Current();
> +    if (slice != NULL) {

Same as above.

@@ +4247,4 @@
>  Collect(JSRuntime *rt, bool incremental, int64_t budget,
>          JSGCInvocationKind gckind, gcreason::Reason reason)
>  {
> +    // GC shouldn't be running in par. exec. mode

/* */

@@ +4730,5 @@
> +}
> +
> +bool
> +ArenaLists::containsArena(JSRuntime *rt,
> +                          ArenaHeader *needle)

Put it all on one line.

@@ +4734,5 @@
> +                          ArenaHeader *needle)
> +{
> +    AutoLockGC lock(rt);
> +    size_t allocKind = needle->getAllocKind();
> +    for (ArenaHeader *header = arenaLists[allocKind].head;

Rename to aheader.

@@ +4736,5 @@
> +    AutoLockGC lock(rt);
> +    size_t allocKind = needle->getAllocKind();
> +    for (ArenaHeader *header = arenaLists[allocKind].head;
> +         header != NULL;
> +         header = header->next) {

If this fits in 99 characters, use one line. Otherwise, the brace should go on its own line.

::: js/src/jsgc.h
@@ +416,5 @@
> +    /* Moves all arenas from |fromArenaLists| into |this|.  In
> +     * parallel blocks, we temporarily create one ArenaLists per
> +     * parallel thread.  When the parallel block ends, we move
> +     * whatever allocations may have been performed back into the
> +     * compartment's main arena list using this function. */

This should have the form:
  /*
   * Text here.
   */

@@ +445,5 @@
>  
> +    // Invoked from IonMonkey-compiled parallel worker threads to
> +    // perform an allocation.  In this case, |this| will be
> +    // thread-local, but the compartment |comp| is shared between all
> +    // threads.

Please use a /* */-style comment.

::: js/src/jsobj.cpp
@@ +2441,5 @@
>  {
> +    size_t oldSize = Probes::objectResizeActive() ? computedSizeOfThisSlotsElements() : 0;
> +
> +    if (!growElements(&cx->compartment->allocator, newcap)) {
> +        JS_ReportOutOfMemory(cx);

This will cause the OOM report to happen twice in some cases, but I guess that's not the end of the world...

@@ +2458,5 @@
> +    // NB: This version of |growElements()|, which takes a
> +    // |js::Allocator*| as opposed to a |JSContext*|, is intended to
> +    // run either during sequential or parallel execution.  As per
> +    // convention, since it does not take a JSContext*, it does not
> +    // report an error on out of memory but simply returns false.

Please use this style of comment:
  /*
   * Comment.
   */
Also, the NB: isn't really needed.

::: js/src/vm/ForkJoin.cpp
@@ +30,4 @@
>      JSContext *const cx_;          // Current context
>      ThreadPool *const threadPool_; // The thread pool.
>      ForkJoinOp &op_;               // User-defined operations to be perf. in par.
> +    const uint32_t numSlices_;    // Total number of threads.

Line up the comment.

@@ +246,3 @@
>      // Notify workers to start and execute one portion on this thread.
>      {
> +        gc::AutoSuppressGC gc(cx_);

Does this do anything? It's only checked from inside Collect, at which point you'll have already triggered a !InParallelSection assertion.

@@ +277,5 @@
> +    for (unsigned i = 0; i < numSlices_; i++)
> +        comp->adoptWorkerAllocator(allocators_[i]);
> +
> +    if (gcRequested_) {
> +        if (!gcCompartment_) {

No braces here or in the else.

@@ +448,5 @@
>      PR_NotifyAllCondVar(rendezvousEnd_);
>  }
>  
> +void
> +ForkJoinShared::abort()

abort() is a standard library function and so the name is kind of confusing when it appears as a bare word (as it does below). Can you rename this to something else?

@@ +605,5 @@
> +    {
> +        // Note: we do not allow GC during parallel sections.
> +        // Moreover, we do not wish to worry about making
> +        // write barriers thread-safe.  Therefore, we guarantee
> +        // that there is no incremental GC in progress:

End with a period.

@@ +619,5 @@
> +    ~AutoEnterParallelSection() {
> +        cx_->runtime->mainThread.ionTop = prevIonTop_;
> +    }
> +};
> +}

Put /* namespace js */ here.

::: js/src/vm/ForkJoin.h
@@ +79,5 @@
> +// In Ion-generated code, we will do allocation through the |Allocator|
> +// found in |ForkJoinSlice| (which is obtained via TLS).  Also, no
> +// write barriers are emitted.  Conceptually, we should never need a
> +// write barrier because we only permit writes to objects that are
> +// newly allocated, and such objects are always black (to use inc. GC

Please spell out incremental here and below.

@@ +109,5 @@
>  struct ForkJoinOp;
>  
> +// Returns the number of slices that a fork-join op will have when
> +// executed.
> +uint32_t ForkJoinSlices(JSContext *cx);

uint32_t should be on its own line.

@@ +123,5 @@
>  class AutoRendezvous;
>  class AutoSetForkJoinSlice;
> +
> +#ifdef DEBUG
> +struct IonTraceData {

Brace should go on its own line.

@@ +154,5 @@
>  
> +    // If we took a parallel bailout, the script that bailed out is stored here.
> +    JSScript *abortedScript;
> +
> +    // Records the last instr. to execute on this thread.

Spell out instruction.

@@ +177,5 @@
>  
> +    // When the code would normally trigger a GC, we don't trigger it
> +    // immediately but instead record that request here.  This will
> +    // cause |ExecuteForkJoinOp()| to invoke |TriggerGC()| or
> +    // |TriggerCompartmentGC()| as appropriate once the par. sec. is

Spell out parallel section.

@@ +180,5 @@
> +    // cause |ExecuteForkJoinOp()| to invoke |TriggerGC()| or
> +    // |TriggerCompartmentGC()| as appropriate once the par. sec. is
> +    // complete. This is done because those routines do various
> +    // preparations that are not thread-safe, and because the full set
> +    // of arenas is not available until the end of the par. sec.

here too

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remove gcMallocAndFreeBytes

This was holding up Niko, so I think we should remove it. I added it in the hopes of eliminating gcMallocBytes, which doesn't account for freed objects. However, I eventually realized that it's very difficult to know the size of the slot arrays that we free when finalizing objects (since the shape has already been swept) and so we'll never be able to make the switch. Therefore it doesn't seem worth keeping.

Comment 14

[Niko Matsakis [:nmatsakis]]

Attachment: Incorporates billm's comments and patch.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=cfd1428415fd

Carrying over r+ from attachment 698833 [details] [diff] [review] and 701196.

Comment 15

[Niko Matsakis [:nmatsakis]]

Attachment: Incorporates billm's comments and patch and dmandelin's suggestion.

Try run: http://tbpl.mozilla.org/?tree=Try&rev=7caf56b9ba58

Carrying over r+ from previous review.

Comment 16

[Niko Matsakis [:nmatsakis]]

Attachment: Correct assertion.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0a8adfb18ff3

Carrying over r+.

Comment 17

[Niko Matsakis [:nmatsakis]]

Attachment: Final patch.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=42557e9911dd

Carry over r+ from previous patch.

Comment 18

[Niko Matsakis [:nmatsakis]]

Attachment: Various minor fixes for ARM build.

Try: https://tbpl.mozilla.org/?tree=Try&rev=186c33cfe963

Carrying over r+

Comment 19

[Niko Matsakis [:nmatsakis]]

Pushed to inbound:  https://hg.mozilla.org/integration/mozilla-inbound/rev/ace1e79d65d9

Comment 20

[Luke Wagner [:luke]]

I'm seeing a ton of warnings (GCC) of the form:
  ../jscntxt.h:1128:18: warning: inline function ‘void* JSRuntime::malloc_(size_t, JSCompartment*, JSContext*)’ used but never defined [enabled by default]

Perhaps you could move the JSRuntime::malloc_ definitions to the end of jscntxt.h so we don't have to #include jscntxtinlines.h from everywhere?

Comment 21

[Niko Matsakis [:nmatsakis]]

Huh, I didn't see those warnings.  Unfortunately, I had to move them into jscntxtinlines.h so that they could reference some field or other as part of an assertion, I think. We could potentially remove this assertion.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

You should be able to move those into jscntxt.h, but put them after the definition of JSContext.

Also, please go through and make sure everything conforms to SM style. I noticed some cases where you had |void* p| instead of |void *p|. Also, braces for functions defined at the top level (i.e., not indented) should go on their own line. This wasn't fixed for the functions that are causing the warnings.

Also, did you see my comment about ForkJoinSlice::InParallelSection? You didn't make the change or say anything about it.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, there are some //-style comments in jscntxt.h. We use only /* */-style comments in that file.

Comment 24

[Niko Matsakis [:nmatsakis]]

OK, I'll prepare an additional style patch.

Comment 25

[Niko Matsakis [:nmatsakis]]

Regarding the name of ForkJoinSlice---yes, I didn't change the name, as that is a pre-existing class, and I interpreted that as more of a suggestion.  Sorry about that.

The current name originated because it was suggested in bug 801087 that I should avoid generic terms like "context" or "data"---"context" in particular---so we settled on ForkJoinSlice.  The intention was that it indicates the slice of the computation that you are currently working on. Anyhow, I will rename it to something else.

Comment 26

[Niko Matsakis [:nmatsakis]]

Attachment: Various style corrections.

This does not include the moving of the methods out of ForkJoinSlice, just the other stuff.  I'll tackle that next, just wanted to silence the warnings ASAP.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=37113c591c78

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 706580 [details] [diff] [review]
Various style corrections.

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

Thanks, Niko.

::: js/src/jscntxt.h
@@ +1364,4 @@
>  }
>  
>  inline void
> +FreeOp::free_(void *p) {

Not your fault, but this brace should be on its own line too. Can you fix?

Comment 28

[Niko Matsakis [:nmatsakis]]

Pushed style corrections:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbf97e656a9

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ace1e79d65d9
https://hg.mozilla.org/mozilla-central/rev/cfbf97e656a9