1948517 - Assertion failure: cx_->hadResourceExhaustion(), at jit/WarpOracle.cpp:206

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Debug stack

for (let i = 0 ; i < 99 ; i++) {
    var x = -0;
    var y = { [x - 1]() {} };
}

(gdb) bt
#0  0x00005555584c9beb in MOZ_CrashSequence (aAddress=0x0, aLine=206) at /home/i32g7900a/shell-cache/js-dbg-64-linux-x86_64-5a1f8c0bb78f/objdir-js/dist/include/mozilla/Assertions.h:263
#1  js::jit::WarpOracle::createSnapshot (this=0x7fffffffc9f0) at /home/i32g7900a/trees/mozilla-central/js/src/jit/WarpOracle.cpp:205
#2  0x0000555558427ae9 in js::jit::CreateWarpSnapshot (cx=0x7ffff6940400, mirGen=0x7ffff5ec7180, script=...) at /home/i32g7900a/trees/mozilla-central/js/src/jit/Ion.cpp:1631
#3  js::jit::IonCompile (cx=0x7ffff6940400, script=..., osrPc=<optimized out>) at /home/i32g7900a/trees/mozilla-central/js/src/jit/Ion.cpp:1738
#4  js::jit::Compile (cx=cx@entry=0x7ffff6940400, script=script@entry=..., osrFrame=osrFrame@entry=0x7fffffffcc58, osrPc=<optimized out>, osrPc@entry=0x7ffff6943a12 "\230") at /home/i32g7900a/trees/mozilla-central/js/src/jit/Ion.cpp:1932
#5  0x0000555558428975 in BaselineCanEnterAtBranch (cx=0x7ffff6940400, script=..., osrFrame=0x7fffffffcc58, pc=0x7ffff6943a12 "\230") at /home/i32g7900a/trees/mozilla-central/js/src/jit/Ion.cpp:2133
/snip

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8f2b32df275c
user:        Iain Ireland
date:        Wed Jan 22 21:44:05 2025 +0000
summary:     Bug 1941947: Handle non-index doubles as property keys r=jandem

Run with --fuzzing-safe --no-threads --ion-eager, compile with AR=ar sh ../configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 5a1f8c0bb78f.

Iain, is bug 1941947 a likely regressor?

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1941947

Comment 2

[Iain Ireland [:iain]]

This is kind of finicky and annoying.

We have this bytecode:

00082:  15  Sub                         # GLOBAL OBJ (x - 1)
00083:  15  ToPropertyKey               # GLOBAL OBJ TOPROPERTYKEY((x - 1))
00084:  15  Lambda [x - 1]() {}         # GLOBAL OBJ TOPROPERTYKEY((x - 1)) FUN
00089:  15  DupAt 1                     # GLOBAL OBJ TOPROPERTYKEY((x - 1)) FUN TOPROPERTYKEY((x - 1))
00093:  15  SetFunName 0                # GLOBAL OBJ TOPROPERTYKEY((x - 1)) FUN
00095:  15  InitElem                    # GLOBAL OBJ

We convert -1 to a property key, which guards that it's an Int32, and then use its value twice: once as the name of the lambda, and a second time as a property key to store the lambda. Our InitElem sees -1 as a property key, and generates a GuardValue for Int32(-1). We generate MIR that looks like this:

...
41 Sub <- Unbox#41, Constant#40 [double] : Double
43 Unbox Sub#42 to Int32 (fallible) : Int32       // ToPropertyKey
44 Constant unnamed function : Object
45 Lambda <- Phi#17, Constant#44 Object
46 SetFunName <- Lambda#45, Unbox#43
47 GuardValue <- Unbox#43                         // InitElem
...

In ApplyTypes, we see that Unbox43 expects a Value, so we box the Sub. We reach the GuardValue, which also expects a boxed input. Since its input is now an unbox of a box, instead of generating a new box, we reach past Unbox43 and use the freshly created Box as the input to GuardValue47.

In GVN, we convert Unbox42 into a ToNumberInt32, which will correctly convert the double output of the Sub into an Int32. However, because of the earlier changes, we don't actually use that value in the GuardValue. The final MIR looks something like this:

38 Sub <- LoadDynamicSlotAndUnbox#76, Constant#37 [double] : Double
39 Box <- Sub#38 : Value
40 ToNumberInt32 <- Sub#38 : Int32
41 Lambda <- Phi#14, Constant#2 : Object
42 SetFunName <- Lambda#41, Box#39
43 GuardValue <- Box#39

When we reach the GuardValue, we bail out, because the input is DoubleValue(-1) where it expects Int32Value(-1). But when we resume in blinterp, we do so after the SetFunName. The resume point captured the output of ToNumberInt32 as the value of the ToPropertyKey. So the baseline IC sees an Int32 and succeeds, we don't change the CacheIR, and we eventually trigger a bailout loop.

All the individual steps here seem pretty reasonable, but they don't all work together nicely. I'm not immediately sure where the best fix is.

Comment 3

[Iain Ireland [:iain]]

Oh, I've just realized that there's an even more finicky requirement here.

We have a DoubleSubResult IC for the Sub, and a GuardToInt32 for the ToPropertyKey. This is surprising, since the result of the Sub feeds directly into the ToPropertyKey. You'd expect that the guard in the ToPropertyKey would fail, and attach a different stub. The reason it works is that the testcase is being run with --ion-eager. The first time we do the sub, we call into the VM, which uses NumberValue and converts -1.0 into Int32Value(-1). When we get back to the top of the loop, we immediately OSR into Ion, execute in Ion until the GuardValue fails, and bail out. The ToPropertyKey IC is never hit in baseline after the first time we attach a stub.

So this bailout loop can't happen in the browser, because we don't use --ion-eager there.

Vaguely reasonable fixes:

1. Don't short-circuit Unbox->Box in type policies. Create the box and let GVN clean it up.
2. When guarding an Int32 in GuardValue, add code to also accept the equivalent double.

It looks like only a small fraction of tests even hit the type policy short-circuiting code, so I'm going to run some performance tests to see whether option 1 above has an impact.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1941947

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1948517: Don't short-circuit boxing in type policies r=jandem

GVN will clean this up eventually. By delaying until GVN, we give MUnbox::foldsTo a chance to fix things up first.

I ran some performance tests. It doesn't look like this makes a difference. There were some medium-confidence regressions in Jetstream, but upon investigation, json-stringify-inspector (which had the largest confidence) doesn't even hit this path. typescript does hit this path, but I took a profile and compared the generated code for the hottest functions, and they all had the exact same LIR, implying that leaving it until GVN will get the same result.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06361b78ade9
Don't short-circuit boxing in type policies r=jandem

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/06361b78ade9

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox137 to wontfix.

For more information, please visit BugBot documentation.