903193 - PJS: Support setting properties on thread-local objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Still brainstorming on what exactly can be made threadsafe.

A first approximation of restrictions:

 - Object must be local to the thread trying to set a property
 - Type for the field already exists in the TypeObject
 - !watched
 - No hooks (setters, Class hooks)

Comment 1

[Shu-yu Guo [:shu]]

Some things that are complicated and cannot be made threadsafe without more work (to be done after the first approximation lands):

 - Defining a property on an object that's part of some other object's prototype chain, due to |PurgeScopeChain|.
 - Setting Array.length seems awfully complicated.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Part 1: Refactor isThreadLocal to ThreadSafeContext

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Part 2: Refactor methods that ensure dense elements

This is in preparation for doing thread-local setelem on native objects. Refactors ensureDenseElement to have a variety that asserts that packedness information in TI would not be changed, since that can't be done thread locally.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Part 3: Add ability to do readonly lookup on Shape tree and threadlocal hashification

Shape tree stuff is lumped in this patch. Mainly adds 2 functionalities:

1. Able to do thread safe, read only lookups on the compartment-wide Shape tree. Since we warm up in PJS anyways, hopefully the shapes we need to do a set/define is already in the tree.

2. Allows dictionary-fication on thread local objects, since then all the Shapes become owned and cloned.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Part 4: Add methods to HeapSlot which assert no write barrier is needed

In preparation for setting thread local properties, which shouldn't trip any barriers.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Part 5: Refactor SetPropertyHelper to have thread local variant

This is the main patch.

The following things aren't threadsafe and are disallowed in the thread local version and return TP_RETRY_SEQUENTIALLY:

- Mutating the compartment-wide Shape tree
- Mutating TI state
- Non-stub hooks
- Script getters/setters

The following asserts (GC is suppressed during fork join sections in PJS for now, when we integrate with GGC we need to have a story for this):

- Tripping GC barriers

Did I miss anything that's possibly unsafe?

The style of refactoring I took is the one agreed upon some time ago: branch on context type instead of virtuals/templates, but let me know if anything can be improved.

Comment 7

[Shu-yu Guo [:shu]]

Also, since the property cache was removed in bug 704356, the following bit could probably be removed:

>        /*
>         * We need to purge the property cache if obj is itself a prototype or
>         * parent scope; purging is not thread-safe.
>         */
>        if (obj->isDelegate())
>            return TP_RETRY_SEQUENTIALLY;

Which is meant to mirror the call to |PurgeScopeChain|. I don't know enough about |PurgeScopeChain| to say for now though.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Part 6: Support setting .length on thread local Arrays

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 805631 [details] [diff] [review]
Part 4: Add methods to HeapSlot which assert no write barrier is needed

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

Cancelling review for this part after a talk with terrence. Going to play around with allocating a fake shadow::Runtime for thread-local objects that has needsBarrier_ = false; that way all barrier code don't need to be touched and everything is a lot nicer.

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 805631 [details] [diff] [review]
Part 4: Add methods to HeapSlot which assert no write barrier is needed

Removing this in favor of a non-local assert of no barriers are needed as part of cx->isThreadLocal. Not worth it to complicate the Barrier classes.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Part 4: Refactor SetPropertyHelper to have thread local variant v2

Asserts that no barrier is needed as part of cx->isThreadLocal, otherwise use the same |set| paths as sequential code.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Part 5: Support setting .length on thread local Arrays

Ditto

Comment 13

[Brian Hackett [Laid off!]]

Comment on attachment 805623 [details] [diff] [review]
Part 2: Refactor methods that ensure dense elements

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

This looks good, but the post-barrier issue is a real problem and I'd like to see a new patch addressing it.

::: js/src/jit/ParallelFunctions.cpp
@@ +176,5 @@
>  
>  JSObject *
>  jit::ExtendArrayPar(ForkJoinSlice *slice, JSObject *array, uint32_t length)
>  {
> +    JSObject::EnsureDenseResult res = array->ensureDenseElementsPacked(slice, 0, length);

Could the other callers of parExtendDenseElements also use this method or other methods?  It seems not, but it's not really clear from the code what's going on, and parExtendDenseElements really needs a comment.

::: js/src/jsobj.h
@@ +614,5 @@
>      }
>  
>      inline void ensureDenseInitializedLength(js::ExclusiveContext *cx,
>                                               uint32_t index, uint32_t extra);
> +    inline void ensureDenseInitializedLengthPacked(js::ThreadSafeContext *cx,

How about ensureDenseInitializedLengthPreservePackedState?

@@ +730,3 @@
>      inline EnsureDenseResult ensureDenseElements(js::ExclusiveContext *cx,
>                                                   uint32_t index, uint32_t extra);
> +    inline EnsureDenseResult ensureDenseElementsPacked(js::ThreadSafeContext *cx,

Ditto.

::: js/src/jsobjinlines.h
@@ +205,5 @@
>          for (js::HeapSlot *sp = elements + initlen;
>               sp != elements + (index + extra);
>               sp++, offset++)
> +        {
> +            /* Post-barriers are unsafe in PJS. */

Post-barriers are also unsafe for ExclusiveContexts.  I don't think this test should be here, and the assertions in init() should allow writes of non-nursery pointers when running on PJS threads.  Using an |if| like this is both inefficient and papers over the problem --- if a PJS thread somehow got its hands on a nursery pointer it could crash after this write without triggering an assert.

Comment 14

[Brian Hackett [Laid off!]]

Comment on attachment 805627 [details] [diff] [review]
Part 3: Add ability to do readonly lookup on Shape tree and threadlocal hashification

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

::: js/src/vm/Shape-inl.h
@@ +79,5 @@
>  }
>  
> +inline Shape *
> +Shape::searchThreadLocal(ThreadSafeContext *cx, Shape *start, jsid id,
> +                         Shape ***pspp, bool adding)

I don't know how this API is supposed to be used correctly.  ShapeTable::search modifies the table differently depending on whether |adding| is set, but in this case the value of that flag is set based on what seems like totally random data (inDictionary() and isThreadLocal()) and whether this call ignored its |adding| parameter or not isn't really communicated back to the caller.

::: js/src/vm/Shape.h
@@ +770,5 @@
> +         * This predicate may be called from off main thread, thus circumvent the
> +         * assert in zone() and get the zone directly.
> +         */
> +        JS::shadow::Zone *shadowZone = JS::shadow::Zone::asShadowZone(base->arenaHeader()->zone);
> +        return shadowZone->needsBarrier();

How about adding a zoneFromAnyThread() to make this easily grep'able, as is done for runtimeFromAnyThread.

@@ +1005,5 @@
>  
> +    /*
> +     * This function is thread safe if every shape in the lineage of |shape|
> +     * is thread local.
> +     */

What are the times when you would reasonably expect this property to hold?

@@ +1676,5 @@
>      }
>  
>      if (start->numLinearSearches() == LINEAR_SEARCHES_MAX) {
>          if (start->isBigEnoughForAShapeTable()) {
> +            if (Shape::hashify((ThreadSafeContext *)cx, start)) {

No need for this cast, and please do not use C or C++ style casts for these contexts anywhere outside utility methods that have the appropriate asserts.

Comment 15

[Shu-yu Guo [:shu]]

Attachment: Part 1: Add zoneFromAnyThread

Adds zoneFromAnyThread and uses that in barrier code, relying on ShadowZone::barrierTracer() to assert current thread accessibility only when a barrier is actually needed.

Comment 16

[Shu-yu Guo [:shu]]

Attachment: Part 2: Refactor isThreadLocal to ThreadSafeContext

Carrying r+

Comment 17

[Shu-yu Guo [:shu]]

Attachment: Part 3: Refactor methods that ensure dense elements

(In reply to Brian Hackett (:bhackett) from comment #13)
> Comment on attachment 805623 [details] [diff] [review]
> Part 2: Refactor methods that ensure dense elements
> 
> Review of attachment 805623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but the post-barrier issue is a real problem and I'd like
> to see a new patch addressing it.
> 
> ::: js/src/jit/ParallelFunctions.cpp
> @@ +176,5 @@
> >  
> >  JSObject *
> >  jit::ExtendArrayPar(ForkJoinSlice *slice, JSObject *array, uint32_t length)
> >  {
> > +    JSObject::EnsureDenseResult res = array->ensureDenseElementsPacked(slice, 0, length);
> 
> Could the other callers of parExtendDenseElements also use this method or
> other methods?  It seems not, but it's not really clear from the code what's
> going on, and parExtendDenseElements really needs a comment.
> 

Yeah, parExtendDenseElements was written during darker days. I converted the use cases and removed it. There's one callsite where I stuck a FIXME that's fixed by a future part (part 7 at the time of this writing) since the fix depends on parts 3-6.

> ::: js/src/jsobj.h
> @@ +614,5 @@
> >      }
> >  
> >      inline void ensureDenseInitializedLength(js::ExclusiveContext *cx,
> >                                               uint32_t index, uint32_t extra);
> > +    inline void ensureDenseInitializedLengthPacked(js::ThreadSafeContext *cx,
> 
> How about ensureDenseInitializedLengthPreservePackedState?

I opted for PackedFlag, 1 less character and the name is already super long.

> 
> @@ +730,3 @@
> >      inline EnsureDenseResult ensureDenseElements(js::ExclusiveContext *cx,
> >                                                   uint32_t index, uint32_t extra);
> > +    inline EnsureDenseResult ensureDenseElementsPacked(js::ThreadSafeContext *cx,
> 
> Ditto.
> 
> ::: js/src/jsobjinlines.h
> @@ +205,5 @@
> >          for (js::HeapSlot *sp = elements + initlen;
> >               sp != elements + (index + extra);
> >               sp++, offset++)
> > +        {
> > +            /* Post-barriers are unsafe in PJS. */
> 
> Post-barriers are also unsafe for ExclusiveContexts.  I don't think this
> test should be here, and the assertions in init() should allow writes of
> non-nursery pointers when running on PJS threads.  Using an |if| like this
> is both inefficient and papers over the problem --- if a PJS thread somehow
> got its hands on a nursery pointer it could crash after this write without
> triggering an assert.

Agreed, I didn't update this patch after I opted to go for non-local asserts that no barriers are needed. This check is removed and goes through the same path as the sequential case.

Comment 18

[Shu-yu Guo [:shu]]

Attachment: Part 4: Add ability to do readonly lookup on Shape tree and threadlocal hashification v2

(In reply to Brian Hackett (:bhackett) from comment #14)
> Comment on attachment 805627 [details] [diff] [review]
> Part 3: Add ability to do readonly lookup on Shape tree and threadlocal
> hashification
> 
> Review of attachment 805627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/Shape-inl.h
> @@ +79,5 @@
> >  }
> >  
> > +inline Shape *
> > +Shape::searchThreadLocal(ThreadSafeContext *cx, Shape *start, jsid id,
> > +                         Shape ***pspp, bool adding)
> 
> I don't know how this API is supposed to be used correctly. 
> ShapeTable::search modifies the table differently depending on whether
> |adding| is set, but in this case the value of that flag is set based on
> what seems like totally random data (inDictionary() and isThreadLocal()) and
> whether this call ignored its |adding| parameter or not isn't really
> communicated back to the caller.
> 

I added better comments and changed the behavior to asserting that the object has a thread local shape if adding = true. The intended use case is the callsite from putProperty in the next part. What's going on there is that putProperty attempts to allocate an entry in the shape tree *if the object has a shape tree of some kind*. That's only thread safe for thread local dictionaries, which is why this was checking for inDictionary() before

> ::: js/src/vm/Shape.h
> @@ +770,5 @@
> > +         * This predicate may be called from off main thread, thus circumvent the
> > +         * assert in zone() and get the zone directly.
> > +         */
> > +        JS::shadow::Zone *shadowZone = JS::shadow::Zone::asShadowZone(base->arenaHeader()->zone);
> > +        return shadowZone->needsBarrier();
> 
> How about adding a zoneFromAnyThread() to make this easily grep'able, as is
> done for runtimeFromAnyThread.
> 

Done as a separate part.

> @@ +1005,5 @@
> >  
> > +    /*
> > +     * This function is thread safe if every shape in the lineage of |shape|
> > +     * is thread local.
> > +     */
> 
> What are the times when you would reasonably expect this property to hold?
> 

During the conversion of an object to dictionary mode. The entire shape lineage is cloned, then hashified.

> @@ +1676,5 @@
> >      }
> >  
> >      if (start->numLinearSearches() == LINEAR_SEARCHES_MAX) {
> >          if (start->isBigEnoughForAShapeTable()) {
> > +            if (Shape::hashify((ThreadSafeContext *)cx, start)) {
> 
> No need for this cast, and please do not use C or C++ style casts for these
> contexts anywhere outside utility methods that have the appropriate asserts.

That cast was there because C++ is stupid. In Shape.h we only have forward decls of the context types, so it can't figure out that ExclusiveContext -> ThreadSafeContext is an upcast. In this version I moved it to Shape-inl.h. I didn't want to do that before, so I just did a cast.

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 807013 [details] [diff] [review]
Part 4: Refactor SetPropertyHelper to have thread local variant v2

Attached the wrong patch...

Comment 20

[Shu-yu Guo [:shu]]

Comment on attachment 807593 [details] [diff] [review]
Part 4: Add ability to do readonly lookup on Shape tree and threadlocal hashification v2

Attached the wrong patch. Can't handle these big patch queues tonight...

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Part 4: Add ability to do readonly lookup on Shape tree and threadlocal hashification v2

Comment 22

[Shu-yu Guo [:shu]]

Attachment: Part 5: Refactor SetPropertyHelper to have thread local variant v2

Comment 23

[Shu-yu Guo [:shu]]

Attachment: Part 6: Support setting .length on thread local Arrays v2

Comment 24

[Shu-yu Guo [:shu]]

Attachment: Part 7: Replace PushPar since parExtendDenseElements is gone

Fixes FIXME introduced in part 3.

Comment 25

[Niko Matsakis [:nmatsakis]]

Comment on attachment 807598 [details] [diff] [review]
Part 7: Replace PushPar since parExtendDenseElements is gone

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

This looks good, though I haven't read the rest of the patch series (on which this patch crucially depends).

Comment 26

[Brian Hackett [Laid off!]]

Comment on attachment 807587 [details] [diff] [review]
Part 1: Add zoneFromAnyThread

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

Sorry for the delay.

::: js/src/gc/Heap.h
@@ +1029,2 @@
>      JS_ASSERT(isTenured());
> +    return zone;

The arenaHeader() call is pretty meaningless if !isTenured().  How about:

JS_ASSERT(isTenured());
return arenaHeader()->zone;

Comment 27

[Brian Hackett [Laid off!]]

Comment on attachment 807590 [details] [diff] [review]
Part 3: Refactor methods that ensure dense elements

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

::: js/src/jit/CodeGenerator.cpp
@@ -4817,5 @@
> -    // of two labels because (at least in parallel mode) we can
> -    // recover from index < capacity but not index !=
> -    // initializedLength.
> -    Label indexNotInitLen;
> -    Label indexWouldExceedCapacity;

Why is this code being removed?  It's pretty crucial that we be able to bump the initialized length inline, and this patch changes things so that we are always making a stub call.

Comment 28

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #27)
> Comment on attachment 807590 [details] [diff] [review]
> Part 3: Refactor methods that ensure dense elements
> 
> Review of attachment 807590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/CodeGenerator.cpp
> @@ -4817,5 @@
> > -    // of two labels because (at least in parallel mode) we can
> > -    // recover from index < capacity but not index !=
> > -    // initializedLength.
> > -    Label indexNotInitLen;
> > -    Label indexWouldExceedCapacity;
> 
> Why is this code being removed?  It's pretty crucial that we be able to bump
> the initialized length inline, and this patch changes things so that we are
> always making a stub call.

The bumping path isn't being removed. What's being removed is that PJS couldn't handle the general case of when we couldn't bump the initialized length, so that path was split into 2: one where it was because we needed to allocate more space (which PJS could handle via the gross parExtendDenseElements), and one where index was just not equal to initialized length (which PJS couldn't handle). If you look at the execution mode switch below, you'll see that for SequentialExecution both labels jumped to the same place.

With this patch we'll be able to call a generalized setelem VM function for thread local objects, there's no need for this 2-path distinction.

Comment 29

[Brian Hackett [Laid off!]]

Comment on attachment 807594 [details] [diff] [review]
Part 4: Add ability to do readonly lookup on Shape tree and threadlocal hashification v2

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

::: js/src/vm/Shape-inl.h
@@ +95,5 @@
> +     * with all its shapes cloned into the current thread, and its shape table
> +     * allocated thread locally. In that case, we may add to the
> +     * table. Otherwise it is not allowed.
> +     */
> +    JS_ASSERT_IF(adding, cx->isThreadLocal(start));

Should this also include && start->inDictionary()?  Otherwise searchNoHashify is ignoring the |adding| parameter.

::: js/src/vm/Shape.cpp
@@ +112,5 @@
> +    JS_ASSERT(cx->isThreadLocal(this));
> +#ifdef DEBUG
> +    if (cx->isExclusiveContext())
> +        assertSameCompartmentDebugOnly(cx->asExclusiveContext(), compartment());
> +#endif

This cruft seems unfortunate.  Could assertSameCompartmentDebugOnly take a ThreadSafeContext instead?

Comment 30

[Brian Hackett [Laid off!]]

Comment on attachment 807590 [details] [diff] [review]
Part 3: Refactor methods that ensure dense elements

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

Oops, OK, thanks.

Comment 31

[Brian Hackett [Laid off!]]

Comment on attachment 807595 [details] [diff] [review]
Part 5: Refactor SetPropertyHelper to have thread local variant v2

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

Sorry for the delay.

I think this patch could do much better in allowing sequential and PJS versions of the define/set property methods.  My basic problem with this patch is that it splits the logic into separate functions which are performing essentially the same operations with different constraints, e.g. DefinePropertyOrElement and DefinePropertyOrElementThreadLocal.  This makes it very error prone to change the logic in these methods and also makes reading and understanding the code much harder due to the separation of much (not all) of the core logic into e.g. DetermineDefinePropertyAction.

We do similar stuff to what you are trying to do already, in methods that are templated on AllowGC such as GetPropertyHelperInline.  This is a method where the fundamental logic is the same between the CanGC and NoGC versions, but data structure types are different and the actual logic is a bit different, e.g. we don't call getters in the NoGC version.

I think this is the model you should use for the define/set methods.  Maybe move ExecutionMode out of jit/ and have a DefinePropertyOrElement<ExecutionMode> and SetPropertyHelper<ExecutionMode> with a traits class like MaybeRooted<AllowGC> to control the data types (like ExclusiveContext vs. ThreadSafeContext).

::: js/src/jsarray.cpp
@@ +673,5 @@
>      return true;
>  }
>  
>  bool
> +js::WouldDefinePastNonwritableLength(js::ThreadSafeContext *cx,

No js:: in ThreadSafeContext.

::: js/src/jsobj.cpp
@@ +2586,5 @@
> +        /* TI state change is not thread safe. */
> +        if (!cx->isExclusiveContext())
> +            return false;
> +        types::MarkObjectStateChange(cx->asExclusiveContext(), obj);
> +    }

This is old stuff that was used by JM+TI but isn't used by Ion, you can just remove this |if| and comment.

@@ +2628,5 @@
> +    if (changed && obj->is<GlobalObject>()) {
> +        if (!cx->isExclusiveContext())
> +            return false;
> +        types::MarkObjectStateChange(cx->asExclusiveContext(), obj);
> +    }

Ditto.  This will also allow making this method infallible again.

Comment 32

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #31)
> Comment on attachment 807595 [details] [diff] [review]
> Part 5: Refactor SetPropertyHelper to have thread local variant v2
> 
> Review of attachment 807595 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay.
> 
> I think this patch could do much better in allowing sequential and PJS
> versions of the define/set property methods.  My basic problem with this
> patch is that it splits the logic into separate functions which are
> performing essentially the same operations with different constraints, e.g.
> DefinePropertyOrElement and DefinePropertyOrElementThreadLocal.  This makes
> it very error prone to change the logic in these methods and also makes
> reading and understanding the code much harder due to the separation of much
> (not all) of the core logic into e.g. DetermineDefinePropertyAction.
> 
> We do similar stuff to what you are trying to do already, in methods that
> are templated on AllowGC such as GetPropertyHelperInline.  This is a method
> where the fundamental logic is the same between the CanGC and NoGC versions,
> but data structure types are different and the actual logic is a bit
> different, e.g. we don't call getters in the NoGC version.
> 
> I think this is the model you should use for the define/set methods.  Maybe
> move ExecutionMode out of jit/ and have a
> DefinePropertyOrElement<ExecutionMode> and SetPropertyHelper<ExecutionMode>
> with a traits class like MaybeRooted<AllowGC> to control the data types
> (like ExclusiveContext vs. ThreadSafeContext).

If we're willing to take the cost of templates, I'd love to go that approach. I've been trying to factor out as much as possible without using templates, on the suggestion from billm and terrence a while ago on not using them for fear of doubling the size of the VM for every path that needed to work in both cases. As a result, we get dirtier code.

Comment 33

[Shu-yu Guo [:shu]]

Ditto for something like MaybeRooted, which I recall was previously rejected on similar grounds as "template creep". But again, I would +1 going that approach if we don't mind the space bloat.

Comment 34

[Shu-yu Guo [:shu]]

Conferred with Terrence. I'm going to rewrite part 5 to be templated -- the duplication seems not too bad in this case.

Comment 35

[Shu-yu Guo [:shu]]

Attachment: Part 5: Refactor SetPropertyHelper to have thread local variant v3

Templatized version.

Comment 36

[Brian Hackett [Laid off!]]

Comment on attachment 810963 [details] [diff] [review]
Part 5: Refactor SetPropertyHelper to have thread local variant v3

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

Looks much better, r=me with fixes below.

::: js/src/jit/CompileInfo.h
@@ +6,5 @@
>  
>  #ifndef jit_CompileInfo_h
>  #define jit_CompileInfo_h
>  
> +#include "jscntxt.h"

What is this include necessary for?

::: js/src/jsobj.cpp
@@ +2484,2 @@
>  {
>      JS_ASSERT(obj->inDictionaryMode());

This should JS_ASSERT(cx->isThreadLocal(this))

@@ +3484,5 @@
>          setter == JS_StrictPropertyStub &&
>          attrs == JSPROP_ENUMERATE &&
> +        (!obj->isIndexed() || (mode == ParallelExecution
> +                               ? !obj->nativeContainsPure(id)
> +                               : !obj->nativeContains(cx->asExclusiveContext(), id))))

You can just always call nativeContainsPure here instead.

@@ +3492,5 @@
>          if (!WouldDefinePastNonwritableLength(cx, obj, index, setterIsStrict, &definesPast))
>              return false;
> +        if (definesPast) {
> +            if (mode == ParallelExecution)
> +                return !setterIsStrict;

This |if| needs a comment.

@@ +3531,5 @@
>              if (!WouldDefinePastNonwritableLength(cx, arr, index, setterIsStrict, &definesPast))
>                  return false;
> +            if (definesPast) {
> +                if (mode == ParallelExecution)
> +                    return !setterIsStrict;

Ditto.

@@ +4131,3 @@
>  bool
> +js_NativeSet(typename ExecutionModeTraits<mode>::ContextType cxArg,
> +             Handle<JSObject*> obj, Handle<JSObject*> receiver,

While you're here, can you make this a js:: method?

::: js/src/jspubtd.h
@@ +251,5 @@
> + * mode, the intention of taking a ThreadSafeContext is clear. For functions
> + * that have similar yet different enough behavior (much like the functions
> + * that templatized on AllowGC), we want the function take one of the derived
> + * context types instead -- JSContext or ForkJoinSlice. Additionally,
> + * sometimes we want speed at the cost of extra code via templates.

This isn't a very good overview comment, it mostly focuses on minutiae without any actual overview.  Maybe something like:

JS can execute either sequentially in parallel (i.e. PJS).  Functions which behave identically in either execution mode can take a ThreadSafeContext, and functions which have similar but not identical behavior between execution modes can be templated on the execution mode.  Such functions use a context parameter type from ExecutionModeTraits indicating whether they are only permitted threadsafe operations, or whether they can have arbitrary side effects.

Also, the last sentence here is unnecessary.  Additional code always has a cost whether templates are involved or not, and the amount of extra code involved here is paltry.

BTW, if we are really concerned about the code size of the engine, we should be reducing it in a disciplined fashion rather than with simplistic rules like "no templates".  i.e. measure and watch for excessive inlining or for lots of generated code resulting from classes like Vector and HashTable.

@@ +254,5 @@
> + * context types instead -- JSContext or ForkJoinSlice. Additionally,
> + * sometimes we want speed at the cost of extra code via templates.
> + */
> +
> +enum ExecutionMode {

Why is this stuff in jspubtd.h and not jscntxt.h?  jspubtd.h is a public header and we don't want people outside the VM using ExecutionMode or the various non-JSContext context classes.

Comment 37

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #36)

> ::: js/src/jspubtd.h
> @@ +251,5 @@
> > + * mode, the intention of taking a ThreadSafeContext is clear. For functions
> > + * that have similar yet different enough behavior (much like the functions
> > + * that templatized on AllowGC), we want the function take one of the derived
> > + * context types instead -- JSContext or ForkJoinSlice. Additionally,
> > + * sometimes we want speed at the cost of extra code via templates.
> 
> This isn't a very good overview comment, it mostly focuses on minutiae
> without any actual overview.  Maybe something like:
> 
> JS can execute either sequentially in parallel (i.e. PJS).  Functions which
> behave identically in either execution mode can take a ThreadSafeContext,
> and functions which have similar but not identical behavior between
> execution modes can be templated on the execution mode.  Such functions use
> a context parameter type from ExecutionModeTraits indicating whether they
> are only permitted threadsafe operations, or whether they can have arbitrary
> side effects.
> 
> Also, the last sentence here is unnecessary.  Additional code always has a
> cost whether templates are involved or not, and the amount of extra code
> involved here is paltry.
> 
> BTW, if we are really concerned about the code size of the engine, we should
> be reducing it in a disciplined fashion rather than with simplistic rules
> like "no templates".  i.e. measure and watch for excessive inlining or for
> lots of generated code resulting from classes like Vector and HashTable.
> 

Yeah that's a good point, this wasn't a good comment.

> @@ +254,5 @@
> > + * context types instead -- JSContext or ForkJoinSlice. Additionally,
> > + * sometimes we want speed at the cost of extra code via templates.
> > + */
> > +
> > +enum ExecutionMode {
> 
> Why is this stuff in jspubtd.h and not jscntxt.h?  jspubtd.h is a public
> header and we don't want people outside the VM using ExecutionMode or the
> various non-JSContext context classes.

I can't put it in jscntxt.h because of it cannot be included in jsobj.h, and JSObject decls need ExecutionMode and the traits to work. What I'll do is put ExecutionMode and ExecutionModeTraits in vm/ExecutionMode.h and have jsobj.h include that.

Thanks for the review!

Comment 38

[Shu-yu Guo [:shu]]

Attachment: Part 6: Add ParallelExecution path to ArraySetLength. (r=?)

Comment 39

[Shu-yu Guo [:shu]]

Attachment: fuzz.patch

Requesting fuzz of a rolled up patch of this bug + bug 901761. Applies to m-c rev 242aa3916310

Comment 40

[Brian Hackett [Laid off!]]

Comment on attachment 812313 [details] [diff] [review]
Part 6: Add ParallelExecution path to ArraySetLength. (r=?)

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

::: js/src/jsarray.cpp
@@ +461,5 @@
> +            return false;
> +    } else {
> +        if (!CanonicalizeArrayLengthValue(cxArg->asJSContext(), value, &newLen))
> +            return false;
> +    }

I think this would be better if CanonicalizeArrayLengthValue was templated on the execution mode and returned false on an object if the context is threadsafe.

@@ +662,5 @@
>      // the long run, with accessors replacing them both internally and at the
>      // API level, just run with this.
> +    RootedShape lengthShape(cxArg, mode == ParallelExecution
> +                            ? arr->nativeLookupPure(id)
> +                            : arr->nativeLookup(cxArg->asJSContext(), id));

It should be fine to always do a nativeLookupPure here.  Arrays will usually only have a single shape, |length|.  Alternatively you could template nativeLookup on the context type and don't do any side effects if the cx is threadsafe (which would allow removing nativeLookupPure).

Comment 41

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #40)
> Comment on attachment 812313 [details] [diff] [review]
> Part 6: Add ParallelExecution path to ArraySetLength. (r=?)
> 
> Review of attachment 812313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsarray.cpp
> @@ +461,5 @@
> > +            return false;
> > +    } else {
> > +        if (!CanonicalizeArrayLengthValue(cxArg->asJSContext(), value, &newLen))
> > +            return false;
> > +    }
> 
> I think this would be better if CanonicalizeArrayLengthValue was templated
> on the execution mode and returned false on an object if the context is
> threadsafe.

Sure, that's fine.

> 
> @@ +662,5 @@
> >      // the long run, with accessors replacing them both internally and at the
> >      // API level, just run with this.
> > +    RootedShape lengthShape(cxArg, mode == ParallelExecution
> > +                            ? arr->nativeLookupPure(id)
> > +                            : arr->nativeLookup(cxArg->asJSContext(), id));
> 
> It should be fine to always do a nativeLookupPure here.  Arrays will usually
> only have a single shape, |length|.  Alternatively you could template
> nativeLookup on the context type and don't do any side effects if the cx is
> threadsafe (which would allow removing nativeLookupPure).

nativeLookupPure is used in sequential contexts too currently for correctness in the ICs, so I'd rather not templatize it on an ExecutionMode, which would be confusing to read.

Comment 42

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 812866 [details] [diff] [review]
fuzz.patch

Nothing bad found.

Comment 43

[Christian Holler (:decoder)]

Comment on attachment 812866 [details] [diff] [review]
fuzz.patch

No issues found either.

Comment 44

[Shu-yu Guo [:shu]]

https://tbpl.mozilla.org/?tree=Try&rev=63bb54ea56c0

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5785358e63
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4941a43066c5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b4142a3ecb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1d12937ff6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4da4d90efca2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e302573f54
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cdbc4812c4ee

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb5785358e63
https://hg.mozilla.org/mozilla-central/rev/4941a43066c5
https://hg.mozilla.org/mozilla-central/rev/b3b4142a3ecb
https://hg.mozilla.org/mozilla-central/rev/5a1d12937ff6
https://hg.mozilla.org/mozilla-central/rev/4da4d90efca2
https://hg.mozilla.org/mozilla-central/rev/f7e302573f54
https://hg.mozilla.org/mozilla-central/rev/cdbc4812c4ee