Open Bug 1487022 Opened 2 years ago Updated 1 year ago

JavaScript implementation of SameValue can lead to repeated bailouts

Categories

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

defect

Tracking

()

REOPENED
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- affected

People

(Reporter: anba, Unassigned)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

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
---
This is even worse, as between each bailout we do recompile the function, and hit the same bailout.
Keywords: perf
Priority: -- → P2
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1:f64]
Assignee: nobody → 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
Assignee: tcampbell → khyperia
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)
(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)
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ffc12842207
Fix repeated bailouts when constant-folding a never-ran 1/0 r=tcampbell
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).
Flags: needinfo?(khyperia) → needinfo?(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.
Flags: needinfo?(tcampbell)
Attachment #9009709 - Attachment description: Bug 1487022 - Fix repeated bailouts when constant-folding a never-ran 1/0 → Bug 1487022 - Fix repeated bailouts when constant-folding a never-ran 1/0.
Status: NEW → ASSIGNED
Whiteboard: [qf:p1:f64] → [qf:p1:f65]
Attachment #9016422 - Attachment description: Bug 1487022 - Convert to int32 rather than assuming int32. → Bug 1487022 - Use MOZ_RELEASE_ASSERT for builtins that take int32 values.
Pushed by tcampbell@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1cf53a430311
https://hg.mozilla.org/mozilla-central/rev/860712299944
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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]
Flags: needinfo?(khyperia)
Backout by shindli@mozilla.com:
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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.
Depends on: 1503890
Flags: needinfo?(khyperia)
Priority: P2 → P3
Whiteboard: [qf:p1:f65]
Attachment #9016422 - Attachment is obsolete: true
The first patch here was replaced by this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1503890
Assignee: khyperia → nobody
You need to log in before you can comment on or make changes to this bug.