979867 - outArray[r] assignment in mapPar should be using UnsafePutElement to avoid parallel write guards

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

In the typed object mapPar, we assign the output element using `outArray[i] = r`. This is logically correct but does not interact well with our typed barriers, which do not take the index being written into account but rather just check whether `outArray` (as a whole) falls within the target region.

These simple-minded write barriers are reasonable because in the end user's kernel code, they are only supplied with (opaque) out ponters that point at one element of the output array, so it is always the case that the out pointer is either entirely writable or entirely not writable.

Probably the simplest work around for this problem is to call UnsafePutElement. This also omits the barrier. Note that in the complex case (where the return value is another typed object), we enter a reflective code path, and this path doesn't have any barriers and just uses direct writes -- this should be safe, though I have no idea if it parallelizes yet.

(At first I was concerned that this reflective path had no barriers, but then I remembered that end users only enter it via a property assignment like `foo[x] = y`, in which case there IS a barrier added to `foo` -- but we do have to be careful to maintain this property.)

Comment 1

[Niko Matsakis [:nmatsakis]]

Updated title to indicate that these parallel write guards have NOTHING to do with GGC write barriers. I need to get my terminology straight.

Comment 2

[Lars T Hansen [:lth]]

Some work will be required to update UnsafePutElements and the inlining of ditto.

Comment 3

[Lars T Hansen [:lth]]

Attachment: Test case

Comment 4

[Lars T Hansen [:lth]]

Attachment: Input for test case

Just run the shell with the .js file as the only argument,  the input file must be in the working directory.

Comment 5

[Lars T Hansen [:lth]]

Attachment: Complete fix

The patch addresses three work items:

- call UnsafePutElements to update the array in the parallel worker code
- implement support for TypedObject in the UnsafePutElements intrinsic
- implement inlining of UnsafePutElements on TypedObject

With this applied, the modified test case runs without bailouts (it has been modified to account for Bug 980386).

I'm uncertain about a couple of things here:

- In jsop_setelem_typed_object (introduced by this patch) I'm not calling
  resumeAfter() because there's no ins to hang that onto.  In this I'm
  following existing code for TypedObject, but I'm not sure whether it's
  right or not.

- That same function is only implemented for unsafe operations; I'd like to
  hear whether it's worthwhile (at this stage) to implement it also for
  safe operations, with bounds checking and all that.  I won't
  be able to test that easily.

(Niko, sorry for continuing to hit on you for reviews but TypedObject is pretty much your domain, I guess.)

Comment 6

[Lars T Hansen [:lth]]

Attachment: Test case

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8386933 [details] [diff] [review]
Complete fix

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

This looks fine to me (with comments below addressed), but I'm not very familiar with the TypedObject code, so I didn't look too closely at it. Leaving that to Niko :)

::: js/src/builtin/TypedObject.cpp
@@ +737,5 @@
> +js::IsTypedObjectArray(JSObject& obj)
> +{
> +    if (!obj.is<TypedObject>())
> +        return false;
> +    TypeDescr& d = obj.as<TypedObject>().typeDescr();

Style nit: TypeDescr &d

::: js/src/jit/IonBuilder.cpp
@@ +7801,5 @@
> +                                      bool racy,
> +                                      MDefinition *object, MDefinition *index, MDefinition *value)
> +{
> +    if (safety != SetElem_Unsafe)
> +        return false;

This will be treated as a fatal error by the callers, AFAICS. Maybe we can check this in the caller and/or turn this into an assert?

@@ +7814,5 @@
> +
> +    if (!storeScalarTypedObjectValue(object, byteOffset, arrayType, false, racy, value))
> +        return false;
> +
> +    //return resumeAfter(ins);

Nit: remove this line.

Comment 8

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8386933 [details] [diff] [review]
Complete fix

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

::: js/src/jit/IonBuilder.cpp
@@ +7795,5 @@
>      return resumeAfter(ins);
>  }
>  
>  bool
> +IonBuilder::jsop_setelem_typed_object(ScalarTypeDescr::Type arrayType,

Since this has only one caller, and seems likely to only have one caller ever, I'd be inlined to just inline it into MCallOptimize. Or perhaps you can merge it with the existing routine that handles setelem with typed objects.

::: js/src/jit/MCallOptimize.cpp
@@ +1483,5 @@
>  bool
> +IonBuilder::elementAccessIsTypedObjectArrayOfScalarType(MDefinition* obj, MDefinition* id,
> +                                                        ScalarTypeDescr::Type *arrayType)
> +{
> +    if (obj->mightBeType(MIRType_String))

Why single out string here? I think lookupTypeDescrSet() will get unhappy if `obj` might not be a typed object.

Comment 9

[Niko Matsakis [:nmatsakis]]

Lars -- per your question about implementing jsop_setelem_typed_object() for safe accesses, it's already done. See setElemTryTypedObject() and setElemTryScalarElemOfTypedObject() in IonBuilder.cpp

Comment 10

[Lars T Hansen [:lth]]

(In reply to Niko Matsakis [:nmatsakis] from comment #8)
> 
> ::: js/src/jit/MCallOptimize.cpp
> @@ +1483,5 @@
> >  bool
> > +IonBuilder::elementAccessIsTypedObjectArrayOfScalarType(MDefinition* obj, MDefinition* id,
> > +                                                        ScalarTypeDescr::Type *arrayType)
> > +{
> > +    if (obj->mightBeType(MIRType_String))
> 
> Why single out string here? I think lookupTypeDescrSet() will get unhappy if
> `obj` might not be a typed object.

Looks like lookupTypeDescrSet() is used throughout as a test for whether the object is a TypedObject, so I'll just propagate that, after guarding that 'obj' is at least an object.

Comment 11

[Lars T Hansen [:lth]]

Attachment: Completed patch

Nits fixed; fixed type guard.  Left the problem of non-unsafe access for another day without moving the code.

Comment 12

[Lars T Hansen [:lth]]

I should note that on my MBP, with this fix the test case runs in 3.8s.  Without the fix it runs in 19s.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af491832ff95

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Kill warning "unsigned vs signed comparison

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8406105 [details] [diff] [review]
Kill warning "unsigned vs signed comparison

Duh, pressed Return at the wrong time :/
So, this patch removes the last warning that was showing up at compilation locally. arrobj->as<TypedObject>().length() returns a signed, idx is unsigned. The length attribute of the TypedObject is always handled as an unsigned value though, so it seems safe to convert it in the comparison (changing the length attribute to contain an unsigned value is a much more massive task).

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the quick review!
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4e4b7c15e0

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e4e4b7c15e0