Closed Bug 1125770 Opened 6 years ago Closed 6 years ago

Backtracking started crashing on kraken-stanford-crypto-aes

Categories

(Core :: JavaScript Engine: JIT, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: h4writer, Assigned: bhackett1024)

References

Details

Attachments

(1 file, 1 obsolete file)

http://www.arewefastyet.com/#machine=28&view=single&suite=kraken&start=1421927078&end=1422097812

Arewefastyet doesn't report numbers on "backtracking" anymore since a subtest started to fail/crash. Tested manually this is test "kraken-stanford-crypto-aes" which segfaults.

On the given graph we have two times it started crashing:

First regression range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7cea69231f77&tochange=920ae6ac243b
6d56dfa4e845	Brian Hackett -- Bug 934502 - Remove unnecessary pushedArgumentSlots, track argument slots explicitly in safepoints, r=jandem.

Range that fixed it again:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5cd5233505fc&tochange=bd9169b924a1
a92e6bed098d	Ryan VanderMeulen -- Backed out changeset 6d56dfa4e845 (bug 934502) for SM(ggc) failures.

Second regression range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ef538bd60b1c&tochange=92582779bc79
629c8aac3ece	Brian Hackett — Bug 934502 - Remove unnecessary pushedArgumentSlots, track argument slots explicitly in safepoints, r=jandem.
4d669523c7dd	Brian Hackett -- Bug 1123011 - Box 'this' values when eval'ing strict scripts from a non-strict Ion script, r=jandem.

The revision are just educated guesses. I didn't bisect, but it seems to make sense.
Attached patch patch (obsolete) — Splinter Review
This is a calling conventions issue.  When an Ion frame calls another one and is constructing, after the callee finishes executing the caller will reload the value it pushed for |this| if the callee returned a primitive.  Bug 934502 changed JIT frame tracing so that the |this| slot is no longer required to hold a value, and might not be marked by a GC within the callee if it is not live there.

The attached patch restores the earlier convention of always marking the |this| value as part of the frame, so that the caller can always assume it is live (and holds a valid |this| value for the frame).  I added some logic to the backtracking allocator so that it can't group vregs spilling to the |this| slot with other vregs that don't; this will prevent the allocator from spilling those other vregs to the |this| slot.
Assignee: nobody → bhackett1024
Attachment #8554644 - Flags: review?(jdemooij)
Attachment #8554644 - Flags: review?(jdemooij) → review+
Attached patch patch v2Splinter Review
The first patch had a bug where |this| values for non-function Ion frames were not being marked.  Fixing this also fixes bug 1125840.
Attachment #8554644 - Attachment is obsolete: true
Attachment #8555345 - Flags: review?(jdemooij)
Duplicate of this bug: 1125840
Attachment #8555345 - Flags: review?(jdemooij) → review+
Thanks. Arewefastyet reports numbers again for kraken-standford-crypto-aes (and shumway, which was also affected).
https://hg.mozilla.org/mozilla-central/rev/05f7505159eb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Duplicate of this bug: 1126507
You need to log in before you can comment on or make changes to this bug.