Closed Bug 1200642 Opened 9 years ago Closed 9 years ago

Improve coverage of simulated OOM testing

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(5 files, 4 obsolete files)

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.
Attached patch 1 - oom-alloc-policy (obsolete) — Splinter Review
Add maybeSimulateOOM() method to all implementations of AllocPolicy.  This does nothing for the ones outside js/src.
Attached patch 2 - oom-vector (obsolete) — Splinter Review
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.
Attachment #8655479 - Attachment description: oom-vector → 2 - oom-vector
Attached patch 3 - oom-hashtable (obsolete) — Splinter Review
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.
Attached patch 4 - fix-oomsSplinter Review
Fix OOM handling issues revealed by previous patches.
Attachment #8655486 - Attachment description: 5 - fix-ooms → 4 - fix-ooms
Attachment #8655470 - Flags: review?(jwalden+bmo)
Attachment #8655479 - Flags: review?(jwalden+bmo)
Attachment #8655484 - Flags: review?(terrence)
Attachment #8655486 - Flags: review?(terrence)
Attachment #8655484 - Flags: review?(terrence) → review+
Attachment #8655486 - Flags: review?(terrence) → review+
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 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.
Attached patch oom-alloc-policy v2 (obsolete) — Splinter Review
Good point, comments added.
Attachment #8655470 - Attachment is obsolete: true
Attachment #8657071 - Flags: review?(jwalden+bmo)
Keywords: leave-open
Attachment #8655486 - Flags: checkin+
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-
Attachment #8657071 - Attachment is obsolete: true
Attachment #8659223 - Flags: review?(jwalden+bmo)
Updated with review comments.
Attachment #8655479 - Attachment is obsolete: true
Attachment #8655479 - Flags: review?(jwalden+bmo)
Attachment #8659224 - Flags: review?(jwalden+bmo)
Updated with review comments.
Attachment #8655484 - Attachment is obsolete: true
Attachment #8659225 - Flags: review+
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 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+
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 on attachment 8665430 [details] [diff] [review]
5 - oom-additional-fixes

Review of attachment 8665430 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #8665430 - Flags: review?(terrence) → review+
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.