875661 - PJS: Refactor a threadsafe context out of ForkJoinSlice and JSContext for allocation VM functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

There are some allocation VM functions that we would like to work both in PJS and sequential JS, such as StringConcat, and extending elements. Originally we could pass an allocator, but until such time that the nursery is integrated in PJS, this no longer suffices. We should factor out a ThreadsafeContext which both JSContext and ForkJoinSlice coerce to that can be used for such paths.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: v0

This patch factors out a ThreadsafeContext that's a prefix of JSContext and ForkJoinSlice and is a MallocProvider. The ad-hoc JSObject::MaybeContext is refactored to use ThreadsafeContext.

The patch also moves the malloc counter into PerThreadData, and modify PerThreadData such that it forms a tree, with the root being the main thread, and the children being the worker threads. When all threads join, we add each thread's malloc counter to its parent's.

Comment 2

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

Comment on attachment 753645 [details] [diff] [review]
v0

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

This looks fine. I guess more work is needed to be able to call NewGCThing with a PerThreadContext?

::: js/src/jsapi.cpp
@@ +722,5 @@
> +PerThreadData::~PerThreadData()
> +{
> +    /* Merge state into our parent. */
> +    if (parent_)
> +        parent_->gcMallocBytes += gcMallocBytes;

Seems like this needs to possibly trigger a GC.

::: js/src/jscntxt.h
@@ +1525,5 @@
>  }
>  
> +class ForkJoinSlice;
> +
> +struct ThreadsafeContext : js::ContextFriendFields,

Could this be called ThreadSafeContext (capital S)? Also, could you add a comment explaining how this is expected to be used?

@@ +1548,5 @@
> +    ForkJoinSlice *toForkJoinSlice();
> +
> +    inline void *onOutOfMemory(void *p, size_t nbytes);
> +    inline void updateMallocCounter(size_t nbytes);
> +    inline void reportAllocationOverflow();

I'm realizing that there's not much point in making these three functions inline. Only updateMallocCounter is at all hot, and it's already out of line.

::: js/src/vm/ForkJoin.cpp
@@ +217,5 @@
>      /////////////////////////////////////////////////////////////////////////
> +    // Per-thread data
> +    //
> +    // Each non-main thread worker thread gets its own state.
> +    Vector<PerThreadData *, 16> perThreadData_;

Why is this needed? Keeping the PerThreadData on the thread stack seemed cleaner to me.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Bill McCloskey (:billm) [on vacation until June 9] from comment #2)
> Comment on attachment 753645 [details] [diff] [review]
> v0
> 
> Review of attachment 753645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine. I guess more work is needed to be able to call NewGCThing
> with a PerThreadContext?
> 
> ::: js/src/jsapi.cpp
> @@ +722,5 @@
> > +PerThreadData::~PerThreadData()
> > +{
> > +    /* Merge state into our parent. */
> > +    if (parent_)
> > +        parent_->gcMallocBytes += gcMallocBytes;
> 
> Seems like this needs to possibly trigger a GC.
> 

Ah yes, good catch.

> ::: js/src/jscntxt.h
> @@ +1525,5 @@
> >  }
> >  
> > +class ForkJoinSlice;
> > +
> > +struct ThreadsafeContext : js::ContextFriendFields,
> 
> Could this be called ThreadSafeContext (capital S)? Also, could you add a
> comment explaining how this is expected to be used?
> 
> @@ +1548,5 @@
> > +    ForkJoinSlice *toForkJoinSlice();
> > +
> > +    inline void *onOutOfMemory(void *p, size_t nbytes);
> > +    inline void updateMallocCounter(size_t nbytes);
> > +    inline void reportAllocationOverflow();
> 
> I'm realizing that there's not much point in making these three functions
> inline. Only updateMallocCounter is at all hot, and it's already out of line.
> 
> ::: js/src/vm/ForkJoin.cpp
> @@ +217,5 @@
> >      /////////////////////////////////////////////////////////////////////////
> > +    // Per-thread data
> > +    //
> > +    // Each non-main thread worker thread gets its own state.
> > +    Vector<PerThreadData *, 16> perThreadData_;
> 
> Why is this needed? Keeping the PerThreadData on the thread stack seemed
> cleaner to me.

Because that's the worker thread's stack. Executing the destructor inside the worker thread will make the merging of the malloc counter into the parent PerThreadData racy. Plus, if we make it possible to trigger a GC as you point out above, that will definitely be unsafe. Keeping it outside lets us wait until the threads join, then do the merging work.

As for the generic newGCThing function, I'll add that and upload a v1 patch and have you re-review.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: v1

Address comments + add threadsafeNewGCThing.

Comment 5

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

Comment on attachment 754082 [details] [diff] [review]
v1

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

This seems okay. I have a couple high-level concerns.

I think the malloc counter might race. When we call ThreadSafeContext::updateMallocCounter, that updates the zone's malloc counter, which is not thread safe as I understand. Also, it still seems like ~PerThreadData will fail to trigger a GC if the counter overflows.

I think it would make sense for ThreadSafeContext::updateMallocCounter to avoid updating the counter for the runtime or the zone. Then, when the thread is finished, we could merge the counters from all the threads into both the runtime and the zone counters. That might cause a GC of course, and that needs to be dealt with. Also, I think this could be made simpler than the current model with the parent pointers. Maybe there could be some code at the end of ForkJoinShared::executeFromWorker that would do it inside the lock? That way we could avoid the dynamic allocation of PerThreadData.

::: js/src/jsapi.cpp
@@ +722,5 @@
> +PerThreadData::~PerThreadData()
> +{
> +    /* Merge state into our parent. */
> +    if (parent_)
> +        parent_->gcMallocBytes += gcMallocBytes;

Using += here seems wrong because the malloc counter counts down from gcMaxMallocBytes.

::: js/src/jscntxt.h
@@ +1559,5 @@
> +
> +    explicit ThreadSafeContext(JSRuntime *rt, PerThreadData *pt, ThreadSafeContextKind kind);
> +
> +    /* Returns NULL when not a JSContext. */
> +    JSContext *toJSContext();

I think that toJSContext() and toForkJoinSlice() could be a little safer. I think a more idiomatic way of doing it (in SpiderMonkey at least) would be as follows:

  bool isContext();
  bool isForkJoinSlice();

  JSContext *context(); // asserts that isContext() is true
  ForkJoinSlice *forkJoinSlice(); // asserts that isForkJoinSlice() is true

@@ +1565,5 @@
> +    /* Returns NULL when not a ForkJoinSlice. */
> +    ForkJoinSlice *toForkJoinSlice();
> +
> +    inline JS::Zone *zone() const;
> +    inline void setCompartment(JSCompartment *comp);

I don't think it's safe to permit access to the zone or the compartment from a ThreadSafeContext. Both of these concepts expect to be accessed on the main thread only. We should instead provide cutouts for the cases that are thread-safe (like GC thing allocation).

::: js/src/jsobj.h
@@ +40,4 @@
>  class Nursery;
>  class Shape;
>  struct StackShape;
> +struct ThreadSafeContext;

This should go in jspubtd.h.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Bill McCloskey (:billm) from comment #5)
> Comment on attachment 754082 [details] [diff] [review]
> v1
> 
> Review of attachment 754082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems okay. I have a couple high-level concerns.
> 
> I think the malloc counter might race. When we call
> ThreadSafeContext::updateMallocCounter, that updates the zone's malloc
> counter, which is not thread safe as I understand. Also, it still seems like
> ~PerThreadData will fail to trigger a GC if the counter overflows.
> 

Perhaps it's better to just make it race. We can keep local counters, but what do we set the gcMaxMallocBytes to be on a per-thread basis? Allow each thread to allocate up to the total before triggering a GC seems bad, at least on some platforms. Allowing each thread to allocate up to something like the total / number of threads seems inflexible.

Comment 7

[Shu-yu Guo [:shu]]

To wit, stack allocating PerThreadData is pretty hard to do if we want to keep the thread-local malloc counters. The main thread is reused as one of the worker threads during PJS execution, and if we go with the naive scheme of allowing each of the N PJS worker to allocate an Nth of the total available gcMallocBytes, the gcMallocBytes needs to be lowered before execution and re-raised after its work finishes. But this restoring of gcMallocBytes needs to be done *before* any of the other worker counters merge their state. Doing this purely with stack-allocated PerThreadData would require another lock to synchronize this ordering and seems more trouble than it's worth.

Comment 8

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

(In reply to Shu-yu Guo [:shu] from comment #6)
> Perhaps it's better to just make it race. We can keep local counters, but
> what do we set the gcMaxMallocBytes to be on a per-thread basis? Allow each
> thread to allocate up to the total before triggering a GC seems bad, at
> least on some platforms. Allowing each thread to allocate up to something
> like the total / number of threads seems inflexible.

All right, I guess it's okay to race. In that case, we don't need a separate gcMallocBytes for each thread, right? We can just use the ones for the runtime and the zone.

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Part 1: Add ThreadSafeContext v2

Makes the malloc counter race again, as we're already racing on it, and forgoes complicating logic and heuristics for how to divvy up available malloc bytes among worker threads, which becomes complicated as in such a tree scheme the child counter must be separate from the master counter, but we are reusing the main thread as one of the worker threads during PJS execution.

Comment 10

[Shu-yu Guo [:shu]]

Attachment: Part 2: Make NewGCThing inlines take ThreadSafeContext

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Part 3: Add asserts to thread unsafe getters

This patch adds asserts for thread unsafe getters such that we either are in a sequential section or that we have locked the JSContext.

Also adds *Unsafe variants. I played with putting all functions which need access to such pointers unsafely (like ParWriteGuard, which guards on a pointer belong to a certain arena list and its not being in the runtime's nursery) under some struct and having Cell, ObjectImpl, etc, declaring that struct as a friend, but it seemed too much hassle for a pointer which you could get to if you *really* wanted to. So in the end, the common getters contain some sanity checks, and the Unsafe variants should make you think twice about using them.

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 763071 [details] [diff] [review]
Part 2: Make NewGCThing inlines take ThreadSafeContext

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

This looks great, just needs a few tweaks for the allowGC stuff commented below.

::: js/src/jscntxt.h
@@ +1564,5 @@
> +    inline js::Nursery &nursery();
> +#endif
> +
> +    /*
> +     * Allocator used when allocating GCThings on this context. If we are the

rm 'the'

::: js/src/jsgc.cpp
@@ +1485,5 @@
>      bool runGC = rt->gcIncrementalState != NO_INCREMENTAL &&
>                   zone->gcBytes > zone->gcTriggerBytes &&
>                   allowGC;
> +
> +    JS_ASSERT_IF(runGC, tcx->allowGC());

Rather than assert this, I think that runGC should just be set to false if !tcx->allowGC().  See NewGCThing.

@@ +1514,5 @@
>  
>              rt->gcHelperThread.waitBackgroundSweepEnd();
>          }
>  
>          if (!allowGC)

This test needs to be modified given the change above.

::: js/src/jsgcinlines.h
@@ +509,3 @@
>  {
>      JS_ASSERT(thingSize == js::gc::Arena::thingSize(kind));
> +    JS_ASSERT_IF(!tcx->allowGC(), !allowGC);

I think this assert is too brittle and should be removed.  It means that many of the methods below, such as js_NewGCShape, can never be called with a non-JSContext even though they take a ThreadSafeContext.  While that won't be a problem for PJS, it will be for other off thread uses like parsing and object deserialization.

Comment 13

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

Comment on attachment 763070 [details] [diff] [review]
Part 1: Add ThreadSafeContext v2

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

Beautiful, thanks.

Comment 14

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

Comment on attachment 763071 [details] [diff] [review]
Part 2: Make NewGCThing inlines take ThreadSafeContext

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

This looks nice. I agree with Brian's changes too (although I don't understand why the |if (!allowGC)| branch needs to change).

::: js/src/jsgc.cpp
@@ +1496,5 @@
>  
>          /*
>           * allocateFromArena may fail while the background finalization still
> +         * run. If we aren't in a fork join, we want to wait for it to finish
> +         * and restart.  However, checking for that is racy as the background

Please use one space after the period to be consistent with the rest of the paragraph.

Comment 15

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

Comment on attachment 763072 [details] [diff] [review]
Part 3: Add asserts to thread unsafe getters

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

::: js/src/ion/ParallelFunctions.cpp
@@ +45,5 @@
>  ion::ParWriteGuard(ForkJoinSlice *slice, JSObject *object)
>  {
>      JS_ASSERT(ForkJoinSlice::Current() == slice);
> +    return !IsInsideNursery(object->runtimeUnsafe(), object) &&
> +           slice->allocator()->arenas.containsArena(slice->runtime(), object->arenaHeaderUnsafe());

It seems like we're adding a lot of *Unsafe functions, and these look like the only callers so far. Could we use slice->runtime() to get the runtime here, and just have arenaHeader() not do the assertion? And then eliminate all the *Unsafe stuff? Or do you anticipate using those in other places?

Comment 16

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #14)
> Comment on attachment 763071 [details] [diff] [review]
> I agree with Brian's changes too (although I don't
> understand why the |if (!allowGC)| branch needs to change).

The block below that reads:

        if (!allowGC)
            return NULL;

        /*
         * We failed to allocate. Run the GC if we haven't done it already.
         * Otherwise report OOM.
         */
        if (runGC)
            break;
        runGC = true;

We shouldn't set runGC to true for a ThreadSafeContext.

Comment 17

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

> We shouldn't set runGC to true for a ThreadSafeContext.

I think we'll have already returned because of the new isForkJoinSlice() test. Maybe we could assert that !isForkJoinSlice() when we get to the runGC = true line to make the intention clear.

Comment 18

[Shu-yu Guo [:shu]]

(In reply to Bill McCloskey (:billm) from comment #17)
> > We shouldn't set runGC to true for a ThreadSafeContext.
> 
> I think we'll have already returned because of the new isForkJoinSlice()
> test. Maybe we could assert that !isForkJoinSlice() when we get to the runGC
> = true line to make the intention clear.

The intention isn't that we get to the runGC = true line if we're in PJS, but that we don't try to call rt->gcHelperThread.waitBackgroundSweepEnd();

Being a ForkJoinSlice, with Brian's comment, will set allowGC = false, so we should never get to that runGC = true line anyways.

Comment 19

[Shu-yu Guo [:shu]]

Attachment: Part 3: Add asserts to thread unsafe getters

Adds predicates for checking zones and nursery/allocator containment to ThreadSafeContext. This does it by moving the *Unsafe getters to the protected namespace and making them befriend ThreadSafeContext.

Comment 20

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

Comment on attachment 763925 [details] [diff] [review]
Part 3: Add asserts to thread unsafe getters

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

I'm really not a fan of these Unsafe methods, even if they're hidden. Could you do what I suggested in comment 15? I don't think there's enough evidence at this point that we'll be calling isInsideNursery and isInsideAllocator in many places while off the main thread.

If you want an isSameZone check, I'd recommend adding it as a method to Cell. Then we wouldn't need any extra accessors.

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Part 3: Add asserts to thread unsafe getters v2

Remove Unsafe getters.

The isInsideZone check is for the ConcatStrings patch.

Comment 22

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

Comment on attachment 765132 [details] [diff] [review]
Part 3: Add asserts to thread unsafe getters v2

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

::: js/src/jscntxt.h
@@ +1566,5 @@
> +    /* GC support. */
> +    inline AllowGC allowGC() const;
> +
> +    template <typename T>
> +    inline bool isInsideZone(T thing) const;

Maybe call it isInsideCurrentZone?

Comment 23

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/76077c8abfcc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40bbd1174f46
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/191bed3002c9

Try: https://tbpl.mozilla.org/?tree=Try&rev=911b154773e2

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/76077c8abfcc
https://hg.mozilla.org/mozilla-central/rev/40bbd1174f46
https://hg.mozilla.org/mozilla-central/rev/191bed3002c9
https://hg.mozilla.org/mozilla-central/rev/9695f620df74
https://hg.mozilla.org/mozilla-central/rev/5eabc0ad7e33