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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

7 months ago
No description provided.
Assignee

Comment 1

7 months ago
Posted 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]
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.
Attachment #9016528 - Flags: review?(nicolas.b.pierron) → review+

Comment 3

7 months ago
Pushed by jwalden@mit.edu:
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
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
Flags: needinfo?(jwalden+bmo)
Assignee

Comment 5

7 months ago
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)

Comment 7

7 months ago
Pushed by jwalden@mit.edu:
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
Assignee

Comment 8

7 months ago
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e08e5aa479a9
https://hg.mozilla.org/mozilla-central/rev/5b5648ea1b5b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.