1336580 - AddSlot ICs fail because of unperformed new script analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Tom Schuster [:evilpie]]

In Google Docs almost every time we fail to attach AddSlot stubs because the group has a newScript, but it isn't analyzed yet. In one test case this happens around 25k times. Jandem says he also observed this on octane typescript.

http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/js/src/jit/CacheIR.cpp#1989

Comment 1

[Jan de Mooij [:jandem]]

We could add a CacheIR instruction to guard on the not-yet-analyzed condition we're checking here... We could also flag the stub as a preliminary object stub (we have a mechanism for this in CacheIR.cpp) and then unlink these stubs later when we attach a different AddSlot stub.

Comment 2

[Tom Schuster [:evilpie]]

P1: Removing the condition checking for this, reduces the number of SetProp failures on Google Sheets from 36k to 8k.

Comment 3

[Andrew Overholt [:overholt]]

(Sorry for the whiteboard noise; Quantum Flow decided we didn't need to track all the CacheIR work independently)

Comment 4

[Jan de Mooij [:jandem]]

See comment 0 - it's not entirely clear to me what would break if we removed this if-statement. I'm wondering if we can just mark this stub as a preliminary stub or whether we need to emit a "group->newScript()->analyzed() == false" guard.

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> See comment 0 - it's not entirely clear to me what would break if we removed
> this if-statement. I'm wondering if we can just mark this stub as a
> preliminary stub or whether we need to emit a
> "group->newScript()->analyzed() == false" guard.

If this if-statement were removed, then once the new script analysis has been performed we would need to make sure this stub has either been removed or deactivated so it can't hit.  With the acquired properties analysis there could be a group change on the object (not a shape change, the comment is buggy) and if that group change is skipped by a stub then down the road type information will be degraded (though behavior should still be correct).

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch

This just adds a guard to check the analysis hasn't been performed yet.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68002ca0a77c
Attach AddSlot stubs before we run the new-script analysis. r=bhackett

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/68002ca0a77c