Closed Bug 1368978 Opened 7 years ago Closed 7 years ago

Intermittent js/src/jit-test/tests/arrays/too-long-array-splice.js | Timeout (code -9, args "")

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: anba)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:product])

Attachments

(1 file)

This is spiking badly recently and already led to one patch being erroneously backed out :(
shu: can you find someone to take a look, also see RyanVM;s comment #3
Flags: needinfo?(shu)
On both nightly & beta, we are currently looping over all the elements in the following loop:
http://searchfox.org/mozilla-central/source/js/src/jsarray.cpp#2948-2953

The test case takes about 30 seconds on my laptop in both cases.

Arai mentioned that originally the test case was doing an OOM.
I can try to bisect when this OOM stopped happening on nightly.
The bisection suggests that the first patch which made this test case no longer OOM is Bug 1362753 (Part 2):

https://hg.mozilla.org/mozilla-central/rev/026c02671403

Is this non-OOMing behaviour expected?  Or do we have a way to not take 30s to delete ~2^32 elements from an array?
Flags: needinfo?(shu)
Flags: needinfo?(jdemooij)
Flags: needinfo?(andrebargull)
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Is this non-OOMing behaviour expected?  Or do we have a way to not take 30s
> to delete ~2^32 elements from an array?

It looks like we can speed this up quite easily:
DeletePropertyOrThrow calls DeleteArrayElement, and DeleteArrayElement has fast path for ArrayObjects. This fast path is always taken for each index in the test case, which means we can adjust the delete range based on the getDenseInitializedLength (which is zero for the test case), and that means we can just skip the whole loop.
Flags: needinfo?(jdemooij)
Flags: needinfo?(andrebargull)
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attached patch bug1368978.patchSplinter Review
DeleteArrayElement is already a no-op for any element above the dense initialized length [1]. And that means can simply skip calling DeleteArrayElement for any index greater than the initialized length. This improves |new Array(4294967295).splice(100)| from 30s to 0.02ms for me.

But making this operation faster doesn't actually help to restore the original intent of the test case, namely that we report an OOM in NativeObject::goodElementsAllocationAmount(). To make sure the test case does what it's supposed to do, I've added |Array.prototype[0] = 0;| which (currently) disables any fast paths in Array.prototype.splice.

Oh, and I've also added a missing CheckForInterrupt() when deleting the array elements in splice(). 

[1] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/js/src/jsarray.cpp#531
Attachment #8890813 - Flags: review?(jdemooij)
Comment on attachment 8890813 [details] [diff] [review]
bug1368978.patch

Review of attachment 8890813 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8890813 - Flags: review?(jdemooij) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/433540884241
Skip non-initialized elements when deleting a property range in Array.prototype.splice. r=jandem
Comment on attachment 8890813 [details] [diff] [review]
bug1368978.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1362753
[User impact if declined]: SM(asan) builds nearly permafailing in CI
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, green on inbound. I've also run this through Try on top of Beta and confirmed that things are green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6345874af02c20dbea831fdb8af1e648df40c39b
[Needs manual test from QE? If yes, steps to reproduce]: None
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just adds a fast path to a function and changes one call site to use it.
[String changes made/needed]: None
Attachment #8890813 - Flags: approval-mozilla-beta?
Comment on attachment 8890813 [details] [diff] [review]
bug1368978.patch

Array.splice improvement to fix failing tests, beta55+
Attachment #8890813 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [stockwell fixed:product]
https://hg.mozilla.org/mozilla-central/rev/433540884241
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Should this one have the "PERF" key word?
You need to log in before you can comment on or make changes to this bug.