1487022 - JavaScript implementation of SameValue can lead to repeated bailouts

Status: RESOLVED WORKSFORME

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

Description

[André Bargull [:anba]]

Test case:
---
function SameValue(x, y) {
    if (x === y) {
        return (x !== 0) || (1 / x === 1 / y);
    }
    return (x !== x && y !== y);
}

function f() {
    var lastIndex = 1;
    for (var i = 0; i < 100000; ++i) {
        if (!SameValue(lastIndex, 0)) { }
    }
}

f();
---

Run with: --no-threads


IONFLAGS=bailout shows repeated bailouts from |divi|:
---
[IonBailouts] Took bailout! Snapshot offset: 70
[IonBailouts]  bailing from bytecode: loopentry, MIR: div [30], LIR: divi [26]
[IonBailouts] Baseline info failure /tmp/a.js:8, inlined into /tmp/a.js:8
---

Comment 1

[Nicolas B. Pierron [:nbp]]

This is even worse, as between each bailout we do recompile the function, and hit the same bailout.

Comment 2

[Ted Campbell [:tcampbell]]

We looked at this in detail today.

We generate a Int32 MDiv which we then hoist out of the loop. This has never been run so we have no IC data to make decisions with. In [1], we interpret no IC data as !seenDouble which seems problematic. As well, the constantDoubleResult seems to get confused and not detect type changes leading to incorrect specialization.

[1] https://searchfox.org/mozilla-central/rev/a23c3959b62cd3f616435e02810988ef5bac9031/js/src/jit/MIR.cpp#2943

Comment 3

[Ashley Hauck [:khyperia]]

Attachment: Bug 1487022 - Fix repeated bailouts when constant-folding a never-ran 1/0.

Comment 4

[Ted Campbell [:tcampbell]]

I'm beginning to think we've got a little off track with scope creep here. Since this isn't critical path for anything, I think we should instead hang a few bugs off of this one that fix individual cleanups we've run in to.

- Replace all stub->noteUnoptimizable with general ICState mechanism for all CacheIR ICs that are stale. Also add a helper to BaselineInspector for the general queries about numOptimizedStubs_ and non-zero numFailures_. We also need to remove ICState::invalid_ while here.
- Unify binaryArithTrySpecialized and binaryArithTrySpecializedOnBaselineInspector.
- The constantDoubleResult bug.

Once those are landed then the final code for this bug should be a lot clearer.

(Sorry about all the iterations.. You've uncovered a lot of bit-rot here and I think it is worth fixing)

Comment 5

[Ashley Hauck [:khyperia]]

(In reply to Ted Campbell [:tcampbell] from comment #4)
> I'm beginning to think we've got a little off track with scope creep here.
> Since this isn't critical path for anything, I think we should instead hang
> a few bugs off of this one that fix individual cleanups we've run in to.
> 
> ...

I was originally going to land the noteUnoptimizable changes in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1448039 (there's a decent bit of refactoring in my change for that)

If you'd like me to merge those two bugs and land one change with all of your suggestions there, that works too. (Agreed on the bit-rot)

Note that I (along with Iain) am currently at cultural onboarding, and there's pretty much zero chance of me getting any coding done this week - I'll get back to this next week. (Emails/bug comments are still fine, but don't expect a quick response)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ffc12842207
Fix repeated bailouts when constant-folding a never-ran 1/0 r=tcampbell

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for various spidermonkey failures, e.g. jit-read-u16-from-mdim-array.js:

https://hg.mozilla.org/integration/autoland/rev/8ab15bf1e8cd11a1f043100d2ef3c51d8ba9aaf6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=usercancel%2Crunnable%2Ctestfailed%2Cbusted%2Cexception%2Cretry&group_state=expanded&selectedJob=204285394&revision=3ffc1284220742b9c09c2b8d62c345114c8bd583

Comment 8

[Ashley Hauck [:khyperia]]

Here's my guess at what's happening for one of the failures (from `../jit-test/jit_test.py dist/bin/js --ion TypedObject/jit-read-u16-from-u16-array.js`):

The type inference loosening here changed something that used to always be an int32 to be a Value. The failures only happen with --ion-eager (which is why I didn't catch it before submitting), which I assume hits this "inspecting a never-ran IC" case more often. Because of this, Value flows through instead of int32, which hits this assert:

https://searchfox.org/mozilla-central/source/js/src/builtin/TypedObject.cpp#2712

There are other failures as well, but they're less obvious with their error message (and may root cause down to the same issue).

Comment 9

[Ted Campbell [:tcampbell]]

The TypedObject code asserts that an Int32 flows in but that isn't really well-formed when it is called from script (the self-hosted code that calls it). Since our heuristic changes use double instead of int32 as default in absence of information (to avoid the bailouts), we in this ion-eager cases can have double 0.0 passed as a TypedObject offset.

It seems like the best solution may be to make these take Number as input so they aren't facing the whims of the JITs.

List of functions that look suspect:
> js::NewDerivedTypedObject
> js::AttachTypedObject
> js::SetTypedObjectOffset
> js::StoreScalardouble::Func
> js::StoreReferenceAny::Func
> js::LoadScalardouble::Func
> js::LoadReferenceAny::Func
> 
> intrinsic_SubstringKernel
> intrinsic_SubstringKernel
> intrinsic_GetErrorMessage
> intrinsic_CreateModuleSyntaxError
> intrinsic_CreateModuleSyntaxError
> intrinsic_FinishBoundFunctionInit
> intrinsic_DefineProperty
> intrinsic_UnsafeSetReservedSlot
> intrinsic_UnsafeGetReservedSlot
> intrinsic_TypedArrayBitwiseSlice
> intrinsic_TypedArrayBitwiseSlice
> intrinsic_WarnDeprecatedStringMethod

Probably put that in a separate patch so we can get others to weigh in on it.

Comment 10

[Ashley Hauck [:khyperia]]

Attachment: Bug 1487022 - Use MOZ_RELEASE_ASSERT for builtins that take int32 values.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1cf53a430311
Use MOZ_RELEASE_ASSERT for builtins that take int32 values. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/860712299944
Fix repeated bailouts when constant-folding a never-ran 1/0. r=tcampbell

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/1cf53a430311
https://hg.mozilla.org/mozilla-central/rev/860712299944

Comment 13

[Stefan Hindli [:stefan_hindli]]

Backed out 2 changesets (Bug 1487022) for causing spidermonkey bustages in /builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/memory.js CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=209011830

https://treeherder.mozilla.org/logviewer.html#?job_id=209011830&repo=autoland&lineNumber=54203

[task 2018-10-31T22:58:16.630Z] /builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/memory.js:171:5 Error: Second argument must be an Int32.. Usage: setJitCompilerOption(<option>, <number>)
[task 2018-10-31T22:58:16.630Z] Stack:
[task 2018-10-31T22:58:16.630Z]   @/builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/memory.js:171:5
[task 2018-10-31T22:58:16.630Z] Exit code: 3
[task 2018-10-31T22:58:16.630Z] FAIL - wasm/memory.js
[task 2018-10-31T22:58:16.630Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/memory.js | /builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/memory.js:171:5 Error: Second argument must be an Int32.. Usage: setJitCompilerOption(<option>, <number>) (code 3, args "--ion-eager --ion-check-range-analysis --ion-extra-checks --no-sse3") [4.1 s]
[task 2018-10-31T22:58:16.630Z] {"action": "test_start", "jitflags": "--ion-eager --ion-check-range-analysis --ion-extra-checks --no-sse3", "pid": 32189, "source": "jittests", "test": "wasm/memory.js", "thread": "main", "time": 1541026692.5203989}
[task 2018-10-31T22:58:16.631Z] {"action": "test_end", "extra": {"jitflags": "--ion-eager --ion-check-range-analysis --ion-extra-checks --no-sse3", "pid": 32189}, "jitflags": "--ion-eager --ion-check-range-analysis --ion-extra-checks --no-sse3", "message": "/builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/memory.js:171:5 Error: Second argument must be an Int32.. Usage: setJitCompilerOption(<option>, <number>)", "pid": 32189, "source": "jittests", "status": "FAIL", "test": "wasm/memory.js", "thread": "main", "time": 1541026696.630838}
[task 2018-10-31T22:58:16.631Z] INFO exit-status     : 3
[task 2018-10-31T22:58:16.631Z] INFO timed-out       : False
[task 2018-10-31T22:58:16.631Z] INFO stderr         2> /builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/memory.js:171:5 Error: Second argument must be an Int32.. Usage: setJitCompilerOption(<option>, <number>)
[task 2018-10-31T22:58:16.631Z] INFO stderr         2> Stack:
[task 2018-10-31T22:58:16.631Z] INFO stderr         2> @/builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/memory.js:171:5
[task 2018-10-31T22:58:16.637Z] TEST-PASS | js/src/jit-test/tests/wasm/stack.js | Success (code 0, args "--no-wasm-baseline") [0.4 s]

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/405215e36bd0
Backed out 2 changesets for causing spidermonkey bustages in /builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/memory.js CLOSED TREE

Comment 15

[Ted Campbell [:tcampbell]]

What are you even doing, IonMonkey?!

In [1] we set ResultType to Double and then try to reduce it to Int32. This causes [2] to take the result of our integer-integer addition and package as a double causing more things to fall down in --ion-eager when we don't have type info.

[1] https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/js/src/jit/MIR.cpp#2939
[2] https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/js/src/jit/MIR.cpp#180

Comment 16

[Ted Campbell [:tcampbell]]

I think this should probably be abandoned for a little while. What should have been a small tweak has shown we have a lot of inconsistency in IonBuilder around inferring types with little data.

Leaving this bug open because the test case is interesting, and the patches we tried are still interesting.

Comment 17

[Ashley Hauck [:khyperia]]

The first patch here was replaced by this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1503890

Comment 18

[André Bargull [:anba]]

Warp prevents repeated bailouts from happening for this test case.