1091795 - Unregress octane-box2d

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1081274 comment 24 through bug 1081274 comment 28.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, so the first step is to answer jandem's questions from bug 1081274 comment 28.

First the getter case, because we only have 12 unoptimizable accesses there.

The first and second unoptimizable accesses are on this bit:

        K.prototype = w.prototype;

in the function defined as:

    F.inherit = function(y, w) {

We fail to attach a stub because oldShape != obj->lastProperty() when we call TryAttachNativeGetPropStub.  That's because apparently we do .prototype on function objects with a resolve hook.

Then we have no unoptimizable get accesses for a while.  Then we get a bunch at:

                        if (f.m_id.key == b.key) {

inside A.prototype.Update.

These happen because IsCacheableGetPropReadSlot returns false: this is an accessor property.  In particular, the getter is scriptable and hasJITCode() returns false for it.  

Then later when we go to ion-compile we have the unoptimizable access flag set, but by that point the getter's hasJITCode() returns true, so we can just go ahead and compile it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Now the setters.  This one is harder, because now we have 3398 unoptimizable accesses (except sometimes 3397 and sometimes 3399).  Except in a debug build we have 6092 unoptimizable accesses, not 3398.

Anyway, I checked and the regression is due entirely to the same thing as the getter case: us deciding we have an unoptimizable access because the scripted setter's hasJITCode() returns false.

The plan: don't note unoptimizable access for temporary conditions like hasJITCode() being false, esp. since Ion will recheck those anyway before trying to optimize.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Unregress octane-box2d by not treating some cases when we can't generate a baseline stub as unoptimizable accesses

In particular, if the access is unoptimizable for temporary reason, like a
scripted accessor not having jitcode compiled yet or an accessor being in the
nursery, we don't want to permanently mark the access spot unoptimizable.  At
some point the accessor will gain jitcode or be tenured and then we can
optimize the access.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Unregress octane-box2d by not treating some cases when we can't generate a baseline stub as unoptimizable accesses

In particular, if the access is unoptimizable for temporary reason, like a
scripted accessor not having jitcode compiled yet or an accessor being in the
nursery, we don't want to permanently mark the access spot unoptimizable.  At
some point the accessor will gain jitcode or be tenured and then we can
optimize the access.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8514531 [details] [diff] [review]
Unregress octane-box2d by not treating some cases when we can't generate a baseline stub as unoptimizable accesses

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

Looks good!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Just out of curiosity, I looked into the other unoptimizable accesses.

1370 of them are:

                b.tangentImpulse = r.m_tangentImpulse;

in L.prototype.Initialize.  These are happening because we're adding a prop, but IsCacheableSetPropAddSlot returns false because we went from 0 dynamic slots to 8.

1193 of them are:

                    b.m_type = E.e_faceA;

in M.CollidePolygons.  These are happening for the same exact reason.

988 of them are:

        this.m_id.key = 0

in R.prototype.Reset.  These are happening because in IsCacheableSetPropCall we have obj->lastProperty() != oldShape.  That's becasuse the setter for the "key" property sets this._key on the object, which presumably adds the _key property and changes the shape.  This is basically bug 1089050.

683 of them are:

            n.manifold = m;

in L.prototype.Initialize.  These are happening because this was a property add that made us go from 0 to 8 dynamic slots.

I'm going to stop now, because I expect most of the rest of these to be in one of those two buckets...

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8529406248

Comment 8

[Boris Zbarsky [:bzbarsky]]

I filed bug 1091978 on having an IC for property adds that need to allocate/reallocate slots.  Assuming that would be useful, of course.

Comment 9

[Carsten Book [:Tomcat]]

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