1301690 - IonMonkey guard for object, when checking the existence of a property.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

The following function comes from react code base, and is the source of repeated bailouts.

n(e,t){
  var n = null === e || e === !1,
      r = null === t || t === !1;
  return n || r || t.ref !== e.ref || "string" == typeof t.ref && t._owner !== e._owner;
}

The comparison  t.ref !== e.ref  bails out as we always guard for object before the MLoadFixedSlot.

Comment 1

[Nicolas B. Pierron [:nbp]]

What we seems to be missing in IonMonkey seems to be the following part of the interpreter:

http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/js/src/vm/Interpreter.cpp#4221-4232

Which returns the prototype of the corresponding primitive type, and then let us do the proper property accesses.

It sounds to me that a proper way to implement this would be to build a new MIR node which coerce primitive type to their corresponding Object type, and create a new temporary type set which aggregates the objects of the initial type set, as well as the objects of the prototypes.

Thus, even if we had a single object in the original ObjectCount of the typeset, we would have more than one, if the result set had any primitive type.

Thus we would likely generate MGetPropertyPolymorphic accesses, which does not seems to support missing property accesses yet.

Comment 2

[Nicolas B. Pierron [:nbp]]

I first tried to convert all primitives types flowing into a jsop_getprop into their primitive type, providing the corresponding type set, and being transparent for existing objects.  After making this patch I noticed 2 things:
 - IonBuilder does not distinguish the object from which the property is read from the receiver in case of a getter.
 - I was constantly producing garbage instruction at the top of jsop_getprop, which would be useless and unnecessary in most of the cases.

As I tried to move this unwrapping instruction, I started to notice that there is a shorter way to solve this issue, which is to prevent any of these optimizations if we have a primitive type registered on the type set, thus falling back on an SharedIC for GetProp, which already handle this case well in Baseline.

As remaining issues we could optimize the case where we have a single primitive type as input, such as string, numbers, symbols and boolean, which is not the case of the react code from comment 0.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Prevent using getPropTryDefiniteSlot when primitives can be used as object.

The result type set of the input object seen in this test case was:
 - 1 object in the object count of the TypeSet
 - A few primitives.

This patch add objectOrSentinel guards for getDefiniteSlot and
getUnboxedOffset, which are used for inlining getprop and setprop for the
known objects.  The type policy of these (and MGuardObject of getprop)
should remain as they ensure that Null and Undefined values would still
bailout, without preventing inlining cases.

This patch causes the code above to use a SharedIC (GetProp_Primitive) when
primitives are reported on the typeset.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8791531 [details] [diff] [review]
IonMonkey: Prevent using getPropTryDefiniteSlot when primitives can be used as object.

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

Sorry for the delay.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d616d2e9d1a4
IonMonkey: Prevent using getPropTryDefiniteSlot when primitives can be used as object. r=jandem

Comment 6

[Carsten Book [:Tomcat]]

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