Closed
Bug 1200642
Opened 9 years ago
Closed 9 years ago
Improve coverage of simulated OOM testing
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(5 files, 4 obsolete files)
8.29 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
7.56 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
4.72 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
6.15 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
3.54 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
We have infrastructure to allow us to test handling of out of memory conditions by simulating OOM in debug builds. This works by arranging to return an error condition from a specific memory allocation (or all allocations after a certain point). We attempt to test all failure paths by running test code in a loop and failing at successive allocation points. However this only tests failure on paths where was allocated in this execution, not all paths where memory might be allocated in a real execution. For example, a vector allocates memory for many elements at the same time. Currently we will only simulate OOM on those vector operations that cause it to allocate. This means that there are many paths where vector operations may fail which are not exercised by our testing. We can improve this situation by allowing our containers to simulate OOM on operations that might have but did not allocate memory.
Assignee | ||
Comment 1•9 years ago
|
||
Add maybeSimulateOOM() method to all implementations of AllocPolicy. This does nothing for the ones outside js/src.
Assignee | ||
Comment 2•9 years ago
|
||
Simuate OOM in vector operations that may allocate memory. Take care not to simulate OOM when growing to less than the inline size N as this is assumed not to fail.
Assignee | ||
Updated•9 years ago
|
Attachment #8655479 -
Attachment description: oom-vector → 2 - oom-vector
Assignee | ||
Comment 3•9 years ago
|
||
Simulate OOM in hashtable operations that may allocate memory. Add putNewInfallible() for populating sets and maps allocated with an initial capacity without the possibility of failure.
Assignee | ||
Comment 4•9 years ago
|
||
Fix OOM handling issues revealed by previous patches.
Assignee | ||
Updated•9 years ago
|
Attachment #8655486 -
Attachment description: 5 - fix-ooms → 4 - fix-ooms
Assignee | ||
Updated•9 years ago
|
Attachment #8655470 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8655479 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8655484 -
Flags: review?(terrence)
Assignee | ||
Updated•9 years ago
|
Attachment #8655486 -
Flags: review?(terrence)
Updated•9 years ago
|
Attachment #8655484 -
Flags: review?(terrence) → review+
Updated•9 years ago
|
Attachment #8655486 -
Flags: review?(terrence) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8655470 [details] [diff] [review] 1 - oom-alloc-policy Review of attachment 8655470 [details] [diff] [review]: ----------------------------------------------------------------- Fine enough, but documentation. ::: mfbt/AllocPolicy.h @@ +83,5 @@ > void reportAllocOverflow() const > { > } > + > + bool maybeSimulateOOM() const This addition requires a corresponding update to the comment near the start of the file. This patch, at least, nowhere clarifies what this method should do. And future readers won't even be seeing this much of a patch.
Attachment #8655470 -
Flags: review?(jwalden+bmo) → review-
Comment 6•9 years ago
|
||
Comment on attachment 8655479 [details] [diff] [review] 2 - oom-vector Review of attachment 8655479 [details] [diff] [review]: ----------------------------------------------------------------- I'm gonna hold off on reviewing this til I have clear documentation in hand for the new method, to measure the patch against that.
Assignee | ||
Comment 7•9 years ago
|
||
Good point, comments added.
Attachment #8655470 -
Attachment is obsolete: true
Attachment #8657071 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Updated•9 years ago
|
Attachment #8655486 -
Flags: checkin+
Comment 10•9 years ago
|
||
Comment on attachment 8657071 [details] [diff] [review] oom-alloc-policy v2 Review of attachment 8657071 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/AllocPolicy.h @@ +47,5 @@ > + * memory (failure simulation in cases where memory *is* allocated is handled > + * in the allocation methods of this class). This allows all possible > + * failure paths to be tested, not just those that actually caused an > + * allocation. > + * The default behaviour should be to always return false. Fallible operations that fail when they return truthy are highly atypical. This should be consistent with everything else, inverted. Also, don't mention SpiderMonkey here -- mfbt is general-purpose and not tied to SpiderMonkey, so features, algorithms, data structures, etc. should be described in generic terms. Putting that together, how about something like: * - bool checkSimulatedOOM() const * Some clients generally allocate memory yet in some circumstances won't * need to do so. For example, appending to a vector with a small amount * of inline storage generally allocates memory, but no allocation occurs * unless appending exceeds inline storage. But for testing purposes, it * can be useful to treat *every* operation as allocating. Clients (such * as this hypothetical append method implementation) should call this * method in situations that don't allocate, but could generally, to * support this. Default behavior should return true; more complicated * behavior might be to return false only after a certain number of * allocations-or-check-simulated-OOMs (coordinating with the other * AllocPolicy methods) have occurred. And then at a use site you might have if (!checkSimulatedOOM()) return false;
Attachment #8657071 -
Flags: review?(jwalden+bmo) → review-
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8657071 -
Attachment is obsolete: true
Attachment #8659223 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 12•9 years ago
|
||
Updated with review comments.
Attachment #8655479 -
Attachment is obsolete: true
Attachment #8655479 -
Flags: review?(jwalden+bmo)
Attachment #8659224 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 13•9 years ago
|
||
Updated with review comments.
Attachment #8655484 -
Attachment is obsolete: true
Attachment #8659225 -
Flags: review+
Comment 14•9 years ago
|
||
Comment on attachment 8659223 [details] [diff] [review] 1 - oom-alloc-policy v3 Review of attachment 8659223 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsalloc.h @@ +101,5 @@ > JS_FRIEND_API(void) reportAllocOverflow() const; > + > + bool checkSimulatedOOM() const { > + if (!js::oom::ShouldFailWithOOM()) > + return true; This negation is confusing, I'd probably do if (should fail with oom) { report oom return false; } return true It feel to me like maybe ShouldFailWithOOM should also invert, somehow, but I can't think of quite the right name if it happened, that wouldn't be confusing the opposite way. Meh, dilatory.
Attachment #8659223 -
Flags: review?(jwalden+bmo) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8659224 [details] [diff] [review] 2 - oom-vector v2 Review of attachment 8659224 [details] [diff] [review]: ----------------------------------------------------------------- This is totally gonna screw up all the outstanding OOM bug oom-after numbers (probably required them to be bumped significantly). So it goes.
Attachment #8659224 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 16•9 years ago
|
||
Fix a few more issues need to make all tests to pass with these patches: - GCRuntime::startTask() needs to release the lock in the fallback path that runs the task on the main thread if it fails to start the task on a helper thread. - Make IonCompile() return an error result if compilation threw and exception - Check for OOM in Linker constructor in baseline compilation - Remove assertion in ~AutoEnterOOMUnsafeRegion() that fails due to OOM testing not currently being threadsafe. I'm working on this separately.
Attachment #8665430 -
Flags: review?(terrence)
Comment 17•9 years ago
|
||
Comment on attachment 8665430 [details] [diff] [review] 5 - oom-additional-fixes Review of attachment 8665430 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #8665430 -
Flags: review?(terrence) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b75024c49a12 https://hg.mozilla.org/integration/mozilla-inbound/rev/c508580bb56d https://hg.mozilla.org/integration/mozilla-inbound/rev/61535c12904e https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2de2c3581a
https://hg.mozilla.org/mozilla-central/rev/b75024c49a12 https://hg.mozilla.org/mozilla-central/rev/c508580bb56d https://hg.mozilla.org/mozilla-central/rev/61535c12904e https://hg.mozilla.org/mozilla-central/rev/3a2de2c3581a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•