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

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

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.
OS: Linux → All
Hardware: x86_64 → All
Posted patch v0 (obsolete) — Splinter Review
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.
Assignee: general → shu
Attachment #753645 - Flags: review?(wmccloskey)
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.
Attachment #753645 - Flags: review?(wmccloskey) → review+
(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.
Posted patch v1 (obsolete) — Splinter Review
Address comments + add threadsafeNewGCThing.
Attachment #753645 - Attachment is obsolete: true
Attachment #754082 - Flags: review?(wmccloskey)
Blocks: 877893
Depends on: 881333
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.
Attachment #754082 - Flags: review?(wmccloskey)
(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.
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.
(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.
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.
Attachment #754082 - Attachment is obsolete: true
Attachment #763070 - Flags: review?(wmccloskey)
Attachment #763071 - Flags: review?(wmccloskey)
Attachment #763071 - Flags: feedback?(bhackett1024)
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.
Attachment #763072 - Flags: review?(wmccloskey)
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.
Attachment #763071 - Flags: feedback?(bhackett1024) → feedback+
Comment on attachment 763070 [details] [diff] [review]
Part 1: Add ThreadSafeContext v2

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

Beautiful, thanks.
Attachment #763070 - Flags: review?(wmccloskey) → review+
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.
Attachment #763071 - Flags: review?(wmccloskey) → review+
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?
(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.
> 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.
(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.
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.
Attachment #763072 - Attachment is obsolete: true
Attachment #763072 - Flags: review?(wmccloskey)
Attachment #763925 - Flags: review?(wmccloskey)
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.
Attachment #763925 - Flags: review?(wmccloskey)
Remove Unsafe getters.

The isInsideZone check is for the ConcatStrings patch.
Attachment #763925 - Attachment is obsolete: true
Attachment #765132 - Flags: review?(wmccloskey)
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?
Attachment #765132 - Flags: review?(wmccloskey) → review+
You need to log in before you can comment on or make changes to this bug.