Closed Bug 1166711 Opened 9 years ago Closed 9 years ago

Add support for unboxed objects in RObjectState.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(15 files, 1 obsolete file)

1.56 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
3.75 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
20.47 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
4.23 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
2.97 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
5.48 KB, patch
Details | Diff | Splinter Review
2.23 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
8.37 KB, patch
Details | Diff | Splinter Review
13.62 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
3.82 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
16.53 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
4.31 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
4.85 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
3.78 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
10.20 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
As commented in bug 1165348 comment 7, Scalar replacement is unabled to remove object allocations as objects allocations which are seen frequently are using unboxed objects.

{M,R}ObjectState should support unboxed object templates and reconstruct the proper unboxed object when requested.
Can you tell me if RNewObject::recover [1] works properly when we have a template object which is an UnboxedPlainObject?  Because from what I understand we explicitly forbid this case in Scalar Replacement [2] code, but we don't do that in MNewObject::canRecoverOnBailout() [3].  So I assume that this is supported.

So, if this is supported, then we would be calling indirectly NewObjectWithGroup, but then we are lacking the initialization of the expando and other fields, as done by UnboxedPlainObject::create [4], right?

What should we do, match the if the template object is a UnboxedPlainObject, and call UnboxedPlainObject::create?  Would this be ok to do that for both mode_ of MNewObject?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Recover.cpp#1172
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/ScalarReplacement.cpp?from=ScalarReplacement.cpp&case=true#119-121
[3] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=MNewObject&case=true#3128-3132
[4] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/UnboxedObject.cpp#659
No longer blocks: 1129313
Flags: needinfo?(bhackett1024)
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> Can you tell me if RNewObject::recover [1] works properly when we have a
> template object which is an UnboxedPlainObject?  Because from what I
> understand we explicitly forbid this case in Scalar Replacement [2] code,
> but we don't do that in MNewObject::canRecoverOnBailout() [3].  So I assume
> that this is supported.
> 
> So, if this is supported, then we would be calling indirectly
> NewObjectWithGroup, but then we are lacking the initialization of the
> expando and other fields, as done by UnboxedPlainObject::create [4], right?
> 
> What should we do, match the if the template object is a UnboxedPlainObject,
> and call UnboxedPlainObject::create?  Would this be ok to do that for both
> mode_ of MNewObject?

I think that you can call NewObjectOperationWithTemplate in both cases, it looks like it will do the same thing as ObjectCreateWithTemplate (albeit a bit slower, though this shouldn't matter).  NewObjectOperationWithTemplate will take care of calling UnboxedPlainObject::create, if necessary.
Flags: needinfo?(bhackett1024)
I apparently cannot catch you frequently on IRC, so I am asking the questions here.

Why do we set the guard flag to MConvertUnboxedObjectToNative, if it is used as part of the pipeline?
I understand that we have a passthrough phase which removes this instruction from the pipeline, but we have no moving phases after, so even if this is not a guard this should still work as expected.
Flags: needinfo?(bhackett1024)
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Why do we set the guard flag to MConvertUnboxedObjectToNative, if it is used
> as part of the pipeline?
> I understand that we have a passthrough phase which removes this instruction
> from the pipeline, but we have no moving phases after, so even if this is
> not a guard this should still work as expected.

The guard/movable stuff on MConvertUnboxedObjectToNative was cargo culted from similar instructions like MConvertElementsToDoubles.  I think the setGuard() was necessary at some point (though maybe not) but in any case these instructions should have a consistent treatment.
Flags: needinfo?(bhackett1024)
Attachment #8610099 - Flags: review?(bhackett1024)
This patch split the Is*Escaped functions such that we can clearly
dinstinguish each loop over the uses of instructions.
Attachment #8610101 - Flags: review?(bhackett1024)
Attachment #8610102 - Flags: review?(bhackett1024)
This patch ensure that if all uses are using MConvertUnboxedObjectToNative,
then the object state would use the native representation of the object.

When we visit the instruction, we emulate the memory manipulation of
MConvertUnboxedObjectToNative, we translate it as a no-op as part of the
MObjectState, as the object would already be converted by the MObjectState
before doing any slot manipulation.
Attachment #8610106 - Flags: review?(bhackett1024)
This patch re-enable the test suite without using a corner case of unboxed
objects. Instead, this patch add "test-join=" to do a cross product with an
additional option, which means that this test case used to be tested with

  --baseline-eager
  --ion-eager

will now be tested with

  --baseline-eager
  --ion-eager
  --baseline-eager --no-unboxed-objects
  --ion-eager --no-unboxed-objects

Which checks that, in cases where unboxed objects are failing, we are still
able to use scalar replacement, as it used to be.

The remaining patches are fixing these test cases.
Attachment #8610107 - Flags: review?(bhackett1024)
This patch add support for unboxed objects to MObjectState. We use a flag
on the MObjectState to record if we have to convert the object to a native
object or if we keep the object as an unboxed object.

We use the order of the properties of the template object to determine the
order of the operands of the MObjectState, which also implies that we can
pull all properties value as we iterate over the properties of the object in
the RObjectState. To improve the time spent looking for the offset, we
create a table which map an offset to its operand index on the MObjectState
instruction.
Attachment #8610112 - Flags: review?(bhackett1024)
This patch ensure that objects which are manipulated with MLoadUnboxedScaled
and MStoreUnboxedScalar are not considered escaped, if the object is the
first property, and if the index is a constant.

Then, when emulating these instructions, we create a new object state, or
pull the value from the last object state.
Attachment #8610113 - Flags: review?(bhackett1024)
Attachment #8610109 - Attachment description: part 2.3 - M{Load,Store}UnboxedScalar: Rename indexType to elementType. r= → part 2.3 - M{Load,Store}UnboxedScalar: Rename indexType to elementType.
Attachment #8610099 - Flags: review?(bhackett1024) → review+
Attachment #8610100 - Flags: review?(bhackett1024) → review+
Attachment #8610101 - Flags: review?(bhackett1024) → review+
Comment on attachment 8610102 [details] [diff] [review]
part 0.4 - Remove large headers from Recover.h.

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

::: js/src/jit/Recover.h
@@ +12,5 @@
> +// Do not include "jit/MIR.h" or others in this file. This file is included by
> +// almost everything. Adding large headers can increase compilation time.
> +//
> +// If you need an enumerated type, just store it as an int on the RClass, and
> +// use the enumerated type in the recover method.

Where is Recover.h included from?  I only see a few .cpp's on tip.
Attachment #8610102 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett (:bhackett) from comment #18)
> Comment on attachment 8610102 [details] [diff] [review]
> part 0.4 - Remove large headers from Recover.h.
> 
> Review of attachment 8610102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/Recover.h
> @@ +12,5 @@
> > +// Do not include "jit/MIR.h" or others in this file. This file is included by
> > +// almost everything. Adding large headers can increase compilation time.
> > +//
> > +// If you need an enumerated type, just store it as an int on the RClass, and
> > +// use the enumerated type in the recover method.
> 
> Where is Recover.h included from?  I only see a few .cpp's on tip.

Hum … right :)

It used to be included in JitFrameIterator.h, which is it-self included in vm/Stack.h, and in vm/Interpreter.h.
Comment on attachment 8610103 [details] [diff] [review]
part 0.5 - Add a big comment to explain Recover instructions.

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

Thanks!

::: js/src/jit/Recover.h
@@ +34,5 @@
> +// Recover instructions are executed either during a bailout, or under a call
> +// when the stack frame is introspected. If the stack is introspected, then any
> +// use of recover instruction must lead to an invalidation of the code, as
> +// resuming the execution might fork observables properties, such as the
> +// object-pointer identity.

This sentence isn't very clear and introduces some new concepts.  Maybe trim this after 'invalidation of the code' ?
Attachment #8610103 - Flags: review?(bhackett1024) → review+
Comment on attachment 8610104 [details] [diff] [review]
part 1.0 - Recover ConvertUnboxedObjectToNative if ObjectGroup match the templateObject group of MNewObject.

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

I don't think we should try to recover ConvertUnboxedObjectToNative.  We shouldn't be generating MConvertUnboxedObjectToNative instructions on objects we just created, because of the huge perf cliff that involves, e.g. bug 1166709.  When that bug is fixed we shouldn't see the cases that this patch is trying to catch anymore, so I don't see why this complexity is necessary.
Attachment #8610104 - Flags: review?(bhackett1024)
Attachment #8610105 - Flags: review?(bhackett1024) → review+
Comment on attachment 8610106 [details] [diff] [review]
part 2.0 - Handle Scalar Replacement of converted unboxed objects.

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

As with patch 1.0, I don't think this one should be necessary once bug 1166709 is fixed.
Attachment #8610106 - Flags: review?(bhackett1024)
Attachment #8610107 - Flags: review?(bhackett1024) → review+
Comment on attachment 8610108 [details] [diff] [review]
part 2.2 - IonBuilder::loadUnboxedProperty: Rename scaledOffset to index.

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

::: js/src/jit/IonBuilder.cpp
@@ +10569,5 @@
> +    // loadUnboxedValue is designed to load any value as if it were contained in
> +    // an array. Thus a property offset is converted to an index, when the
> +    // object is reinterpreted as an array of properties of the same size.
> +    size_t scale = UnboxedTypeSize(unboxedType);
> +    size_t index = offset / scale;

|scale| is only used once, so I think the UnboxedTypeSize would read better if it was folded into the computation of |index|.
Attachment #8610108 - Flags: review?(bhackett1024) → review+
Comment on attachment 8610109 [details] [diff] [review]
part 2.3 - M{Load,Store}UnboxedScalar: Rename indexType to elementType.

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

Eh, I think the distinction between how elements are indexed and how they are read is better with this being |indexType|.  Maybe ask bbouvier to review this?  The fields could definitely use a comment to better explain what's going on though.
Attachment #8610109 - Flags: review?(bhackett1024)
Comment on attachment 8610112 [details] [diff] [review]
part 2.4 - MObjectState: Add support for encoding unboxed objects.

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

::: js/src/jit/Recover.cpp
@@ +1416,5 @@
> +            if (val.isUndefined())
> +                continue;
> +
> +            if (!object->as<UnboxedPlainObject>().setValue(cx, properties[i], val))
> +                return false;

This should use JS_ALWAYS_TRUE instead of being fallible.  If setValue() fails then it does not indicate an OOM but rather that the unboxed property cannot hold the value being set.

@@ +1422,5 @@
> +    } else {
> +        if (object->is<UnboxedPlainObject>()) {
> +            if (!UnboxedPlainObject::convertToNative(cx, object))
> +                return false;
> +        }

I think this block is unnecessary if MConvertUnboxedObjectToNative is not recoverable.

::: js/src/jit/ScalarReplacement.cpp
@@ +190,1 @@
>          }

Ditto.

@@ +362,5 @@
> +
> +            isUnboxed = !def->isConvertUnboxedObjectToNative();
> +            break;
> +        }
> +    }

Ditto.
Attachment #8610112 - Flags: review?(bhackett1024) → review+
Comment on attachment 8610113 [details] [diff] [review]
part 2.5 - ScalarReplacement: Replace M{Store,Load}UnboxedScalar by an offset on the MObjectState.

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

Unboxed objects can also be accessed by {Load,Store}UnboxedObjectOrNull and {Load,Store}UnboxedString.  Handling those can I think be commoned with handling of these two instructions though.

::: js/src/jit/ScalarReplacement.cpp
@@ +678,5 @@
>      ins->setIncompleteObject();
>  }
>  
> +size_t
> +getOffsetOf(MDefinition* index, Scalar::Type type, int32_t baseOffset)

The naming of this method doesn't match SM style.
Attachment #8610113 - Flags: review?(bhackett1024) → review+
Comment on attachment 8610109 [details] [diff] [review]
part 2.3 - M{Load,Store}UnboxedScalar: Rename indexType to elementType.

(In reply to Brian Hackett (:bhackett) from comment #24)
> Eh, I think the distinction between how elements are indexed and how they
> are read is better with this being |indexType|.  Maybe ask bbouvier to
> review this?  The fields could definitely use a comment to better explain
> what's going on though.

When I read |indexType|, I understand it as the type of the index, and not the scale factor related to the size of each element of the array.  In C, this naming issue would be similar to:

  unit8_t* object = …;
  int32_t index = …;
  elementType e = ((elementType*) (object + offsetAdjustment))[index];
                        ^
            elementType / indexType?
Attachment #8610109 - Flags: review?(benj)
If we omit part 1.0, and part 2.0, then I cannot land part 2.1 yet.
Marking Bug 1166709 as a blocker.
Depends on: 1166709
Attachment #8611274 - Flags: review?(bhackett1024) → review+
Comment on attachment 8611275 [details] [diff] [review]
part 2.7 - ScalarReplacement: Replace M{Store,Load}UnboxedString by an offset on the MObjectState.

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

Can more of this be commoned with the UnboxedObjectOrNull changes?
Attachment #8611275 - Flags: review?(bhackett1024) → review+
Comment on attachment 8610109 [details] [diff] [review]
part 2.3 - M{Load,Store}UnboxedScalar: Rename indexType to elementType.

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

I think indexType is confusing indeed, but elementType makes me think of elements, which makes me think of typed arrays, although this is used by TO and unboxed objects too. So it doesn't sound really better.

Thinking out loud:
- for typed arrays, this is the type of the array elements, independently of the type of the value being read/written.
- for typed / unboxed objects, this is the type of the stored value, which should .
In both cases, we're talking about the storage type, which is a distinct concept from the way it is read/written (aka readType/writeType, which are the SIMD weirdos).

What do you think about renaming it storageType, then?
Attachment #8610109 - Flags: review?(benj)
(In reply to Benjamin Bouvier [:bbouvier] from comment #32)
> What do you think about renaming it storageType, then?

This sounds good to me.
The test case of part 2.1 no longer works after rebasing, because where we used to have a MCovertUnboxToNative for the object allocated by the Next function, we now have property accesses such as MSetPropertyCache and MSetPropertyPolymorphic.

These instruction prevent any optimization from Scalar Replacement to work effectively, and thus to provide the speed-up seen in Bug 1165348 comment 7.
Flags: needinfo?(bhackett1024)
(In reply to Nicolas B. Pierron [:nbp] from comment #34)
> The test case of part 2.1 no longer works after rebasing, because where we
> used to have a MCovertUnboxToNative for the object allocated by the Next
> function, we now have property accesses such as MSetPropertyCache and
> MSetPropertyPolymorphic.

The problem is the following:

MNewObject has a NativeObject with the right properties.

object 0x7fffee86e550 from global 0x7fffee85b060 [global]
class 0x159b0e0 Object
flags:
proto <Object at 0x7fffee85c020>
properties:
    ((js::Shape*) 0x7fffee882c90) enumerate JSString* (0x7fffee81cbf8) = Latin1Char * (0x7fffee81cc00) = "value"
: slot 0 = undefined
    ((js::Shape*) 0x7fffee882cb8) enumerate JSString* (0x7fffee81ae98) = Latin1Char * (0x7fffee81aea0) = "done"
: slot 1 = undefined

When apparently when we attempt at making a MStoreFixedSlot under js::jit::IonBuilder::setPropTryDefiniteSlot, we query js::jit::IonBuilder::getDefiniteSlot which fails because the property "value" is not a definiteProperty().
As replied on IRC, this issue is solved by the second part of Bug 1166709.
Flags: needinfo?(bhackett1024)
Comment on attachment 8613637 [details] [diff] [review]
part 2.3 - M{Load,Store}UnboxedScalar: Rename indexType to storageType.

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

Thanks, looks good to me.
Attachment #8613637 - Flags: review?(benj) → review+
Nice. This gives a 5% improvement on octane-raytrace:
http://arewefastyet.com/regressions/#/bug/1166711
Depends on: 1175233
Depends on: 1175350
Depends on: 1175397
No longer depends on: 1174547
No longer depends on: 1175397
Depends on: 1177153
Depends on: 1182060
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: