Closed Bug 1336580 Opened 3 years ago Closed 3 years ago

AddSlot ICs fail because of unperformed new script analysis

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: evilpie, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf-])

Attachments

(1 file)

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
Summary: We don't attach AddSlot ICs because of new script analysis → AddSlot ICs fail because of unperformed new script analysis
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.
Blocks: 1326344
P1: Removing the condition checking for this, reduces the number of SetProp failures on Google Sheets from 36k to 8k.
Priority: -- → P1
Whiteboard: [qf:p1]
(Sorry for the whiteboard noise; Quantum Flow decided we didn't need to track all the CacheIR work independently)
Whiteboard: [qf:p1] → [qf-]
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.
Flags: needinfo?(bhackett1024)
(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).
Flags: needinfo?(bhackett1024)
Attached patch PatchSplinter Review
This just adds a guard to check the analysis hasn't been performed yet.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8857421 - Flags: review?(bhackett1024)
Attachment #8857421 - Flags: review?(bhackett1024) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68002ca0a77c
Attach AddSlot stubs before we run the new-script analysis. r=bhackett
https://hg.mozilla.org/mozilla-central/rev/68002ca0a77c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.