AddSlot ICs fail because of unperformed new script analysis

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: evilpie, Assigned: jandem)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf-])

Attachments

(1 attachment)

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
(Assignee)

Comment 1

7 months ago
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

Updated

6 months ago
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-]
(Assignee)

Comment 4

5 months ago
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)
(Assignee)

Comment 6

4 months ago
Created attachment 8857421 [details] [diff] [review]
Patch

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+

Comment 7

4 months ago
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

Comment 8

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68002ca0a77c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.