Closed Bug 979867 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: nmatsakis, Assigned: lth)

References

Details

Attachments

(4 files, 2 obsolete files)

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.)
Summary: outArray[r] assignment in mapPar should be using UnsafePutElement to avoid write barriers → outArray[r] assignment in mapPar should be using UnsafePutElement to avoid parallel write guards
Updated title to indicate that these parallel write guards have NOTHING to do with GGC write barriers. I need to get my terminology straight.
Blocks: PJS
Some work will be required to update UnsafePutElements and the inlining of ditto.
Assignee: nobody → lhansen
Attached file Test case (obsolete) —
Attached image 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.
Blocks: 980386
Attached patch Complete fix (obsolete) — Splinter Review
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.)
Attachment #8386933 - Flags: review?(nmatsakis)
Attachment #8386933 - Flags: review?(jdemooij)
Attached file Test case
Attachment #8386128 - Attachment is obsolete: true
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.
Attachment #8386933 - Flags: review?(jdemooij) → review+
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.
Attachment #8386933 - Flags: review?(nmatsakis) → review+
Lars -- per your question about implementing jsop_setelem_typed_object() for safe accesses, it's already done. See setElemTryTypedObject() and setElemTryScalarElemOfTypedObject() in IonBuilder.cpp
(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.
Attached patch Completed patchSplinter Review
Nits fixed; fixed type guard.  Left the problem of non-unsafe access for another day without moving the code.
Attachment #8386933 - Attachment is obsolete: true
I should note that on my MBP, with this fix the test case runs in 3.8s.  Without the fix it runs in 19s.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/af491832ff95
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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).
Attachment #8406105 - Flags: review?(lhansen)
Attachment #8406105 - Flags: review?(lhansen) → review+
You need to log in before you can comment on or make changes to this bug.