Filed by: cbook [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=103288419&repo=mozilla-inbound https://queue.taskcluster.net/v1/task/beAfFCzqQc6Z4nG9lQW72w/runs/0/artifacts/public/logs/live_backing.log

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

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?

(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.

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

Comment on attachment 8890813 [details] [diff] [review] bug1368978.patch Review of attachment 8890813 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

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

Comment on attachment 8890813 [details] [diff] [review] bug1368978.patch Array.splice improvement to fix failing tests, beta55+

Should this one have the "PERF" key word?