Closed Bug 1498458 Opened 4 years ago Closed 4 years ago

Properly report OOM on failure to allocate ArrayBuffer contents for a fresh ArrayBuffer that can't fit in inline storage


(Core :: JavaScript: Standard Library, enhancement)

Not set



Tracking Status
firefox64 --- fixed


(Reporter: Waldo, Assigned: Waldo)



(1 file)

No description provided.
Attached patch PatchSplinter Review
I stumbled across this during Intl work.  I'd probably try to reduce the toLocaleString to something simpler, but for the life of me I can't find some simpler op (like assigning to an element on the typed array or something) that doesn't make the OOM failure not get triggered by oomTest.  If you know what's up and can suggest something simpler, let me know, but I've overthought this already, so a comment explaining the unimportance of the toLocaleString is good enough IMO.
Attachment #9016528 - Flags: review?(nicolas.b.pierron)
Comment on attachment 9016528 [details] [diff] [review]

Review of attachment 9016528 [details] [diff] [review]:

I honestly do not know why OOMTest function seems to sometimes skip some allocations, my best guess would be that we compile once at the expected location and fail to make it fail as the expected location.

Also for the test case complexity, I would not mind much unless this increase a lot the time taken by the test case.
Attachment #9016528 - Flags: review?(nicolas.b.pierron) → review+
Pushed by
Properly report OOM on failure to allocate ArrayBuffer contents for a fresh ArrayBuffer that can't fit in inline storage.  r=nbp
Backed out for causing SM build bustages on oom-allocating-arraybuffer-contents.

Push with failures:,testfailed,busted,exception&fromchange=f7c32e7abf7293b99c6d1941af6e15ebc3119d5c&tochange=3ee73bef9537d1d2f09d397150238523be4f6891&searchStr=spider&selectedJob=205220746

Failure log:

Backout link:

[task 2018-10-13T01:33:30.915Z] TEST-PASS | js/src/jit-test/tests/wasm/spec/conversions.wast.js | Success (code 0, args "--no-wasm-baseline") [1.5 s]
[task 2018-10-13T01:33:30.915Z] {"action": "test_start", "jitflags": "--no-wasm-baseline", "pid": 26385, "source": "jittests", "test": "wasm/spec/conversions.wast.js", "thread": "main", "time": 1539394409.431116}
[task 2018-10-13T01:33:30.916Z] {"action": "test_end", "extra": {"jitflags": "--no-wasm-baseline", "pid": 26385}, "jitflags": "--no-wasm-baseline", "message": "Success", "pid": 26385, "source": "jittests", "status": "PASS", "test": "wasm/spec/conversions.wast.js", "thread": "main", "time": 1539394410.915756}
[task 2018-10-13T01:33:30.923Z] Exit code: -6
[task 2018-10-13T01:33:30.924Z] TIMEOUT - typedarray/oom-allocating-arraybuffer-contents.js
[task 2018-10-13T01:33:30.924Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/typedarray/oom-allocating-arraybuffer-contents.js | Timeout (code -6, args "--ion-eager --ion-offthread-compile=off") [150.1 s]
[task 2018-10-13T01:33:30.924Z] {"action": "test_start", "jitflags": "--ion-eager --ion-offthread-compile=off", "pid": 15614, "source": "jittests", "test": "typedarray/oom-allocating-arraybuffer-contents.js", "thread": "main", "time": 1539394260.8456311}
[task 2018-10-13T01:33:30.924Z] {"action": "test_end", "extra": {"jitflags": "--ion-eager --ion-offthread-compile=off", "pid": 15614}, "jitflags": "--ion-eager --ion-offthread-compile=off", "message": "Timeout", "pid": 15614, "source": "jittests", "status": "FAIL", "test": "typedarray/oom-allocating-arraybuffer-contents.js", "thread": "main", "time": 1539394410.924226}
[task 2018-10-13T01:33:30.924Z] INFO exit-status     : -6
[task 2018-10-13T01:33:30.924Z] INFO timed-out       : 0:00:00.078597
[task 2018-10-13T01:33:30.931Z] Exit code: -6
[task 2018-10-13T01:33:30.931Z] TIMEOUT - typedarray/oom-allocating-arraybuffer-contents.js
Flags: needinfo?(jwalden+bmo)
Uh...nbp?  This oomTest is not doing gobs of stuff, and there's not much that could be done to simplify it.  What are my options for adding this test somehow or other, without it timing out in production for some strange reason?  Do I have any?
Flags: needinfo?(jwalden+bmo) → needinfo?(nicolas.b.pierron)
I see 3 options:
 - Do not commit the test case.
 - Mark it as slow.
 - Use a global counter to quit() the test case if the oomTest function re-loop to frequently after the expected failure.
Flags: needinfo?(nicolas.b.pierron)
Pushed by
Properly report OOM on failure to allocate ArrayBuffer contents for a fresh ArrayBuffer that can't fit in inline storage.  r=nbp
Add a test for OOM on failure to allocate ArrayBuffer contents that won't fit in a typed array's inline storage.  r=nbp
I did a little more work to reduce, and I got it down to just a call to |new ArrayBuffer(256)| -- plus resolving ArrayBuffer and calling the tested function once, to reduce the stuff that has to occur inside the OOM-test.

+// |jit-test| skip-if: !('oomTest' in this)
+// Resolve ArrayBuffer before OOM-testing, so OOM-testing runs less code and is
+// less fragile.
+var AB = ArrayBuffer;
+function f()
+  return new AB(256);
+// Create |f|'s script before OOM-testing for the same reason.

In my local debugging, the |iteration| variable inside the implementation of |oomTest| -- that I think corresponds to the number of allocations before simulated OOM -- drops down all the way to 3 when the AllocateArrayBufferContents failure is handled, versus ~1000-1100 with the prior arrangement.  That looks like it should (and, in try-pushes, does) eliminate timeouts.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.