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

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

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.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9016528 [details] [diff] [review]
Patch

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c32e7abf72
Properly report OOM on failure to allocate ArrayBuffer contents for a fresh ArrayBuffer that can't fit in inline storage.  r=nbp

Comment 4

[Cosmin Sabou [:CosminS]]

Backed out for causing SM build bustages on oom-allocating-arraybuffer-contents.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=success,testfailed,busted,exception&fromchange=f7c32e7abf7293b99c6d1941af6e15ebc3119d5c&tochange=3ee73bef9537d1d2f09d397150238523be4f6891&searchStr=spider&selectedJob=205220746

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205220746&repo=mozilla-inbound&lineNumber=90086

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee73bef9537d1d2f09d397150238523be4f6891

[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

Comment 5

[Jeff Walden [:Waldo]]

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?

Comment 6

[Nicolas B. Pierron [:nbp]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e08e5aa479a9
Properly report OOM on failure to allocate ArrayBuffer contents for a fresh ArrayBuffer that can't fit in inline storage.  r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5648ea1b5b
Add a test for OOM on failure to allocate ArrayBuffer contents that won't fit in a typed array's inline storage.  r=nbp

Comment 8

[Jeff Walden [:Waldo]]

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.
+f();
+
+oomTest(f);

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.

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/e08e5aa479a9
https://hg.mozilla.org/mozilla-central/rev/5b5648ea1b5b