1228404 - Add OOM tests for module parsing and evaluation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: test-module-eval-oom

I added an OOM tests for module initialisation.  This brought up a couple of things:

1) As flagged in the review, getStaticName() can fail while compiling a GETIMPORT instruction in Ion, in this case because we hit OOM when adding type information.  I added a fallback to compile a slot load in this case.

2) We were hitting OOM appending to a RecompileInfoVector while bailing out, and the bailout code didn't tolerate this.  I changed RecompileInfoVector to have a single element of inline storage and made the places where we append a single element infallible so this doesn't happen any more.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: fix-backtracking-oom

And this needs one more fix to make the test pass on Windows - we need to ensure sufficient allocator ballast in a loop in the backtracing register allocator.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8692612 [details] [diff] [review]
test-module-eval-oom

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

I'd like an explanation of infallibleAppend changes.

::: js/src/jit/Ion.cpp
@@ +3108,5 @@
>  {
>      JitSpew(JitSpew_IonInvalidate, " Invalidate IonScript %p: %s", this, reason);
>      RecompileInfoVector list;
> +    MOZ_ALWAYS_TRUE(list.reserve(1));
> +    list.infallibleAppend(recompileInfo());

I don't understand this change.

@@ +3142,5 @@
>  
>      RecompileInfoVector scripts;
>      MOZ_ASSERT(script->hasIonScript());
> +    MOZ_ALWAYS_TRUE(scripts.reserve(1));
> +    scripts.infallibleAppend(script->ionScript()->recompileInfo());

I don't understand this change either.

::: js/src/jit/IonBuilder.cpp
@@ +8378,5 @@
> +            rvalType = MIRType_Value;
> +
> +        if (!loadSlot(obj, shape->slot(), NumFixedSlots(targetEnv), rvalType, barrier, types))
> +            return false;
> +    }

This block seems to copied from the bottom of getStaticName. Could you refactor them? The getStaticName version has some more micro optimizations.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8693502 [details] [diff] [review]
fix-backtracking-oom

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

::: js/src/jit/BacktrackingAllocator.cpp
@@ +1135,5 @@
>              LiveRange* range = LiveRange::get(*iter);
>              LiveBundle* bundle = range->bundle();
>              if (range == bundle->firstRange()) {
> +                if (!alloc().ensureBallast())
> +                    return false;

I don't have expertise to review this really, even though it's such a small change. Why does this need to be in a loop? If it's needed here, why does not every use of new(alloc()) require a ensureBallast?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Shu-yu Guo [:shu] from comment #2)

> I'd like an explanation of infallibleAppend changes.

The change to the RecompileInfoVector typedef means that there is always inline storage for one element in the vector.  Hence we can now call infallibleAppend() when appending a single element to one of these vectors, although we still need to call reserve() first.  I'll some add comments to make it clear what's happening.

> > +                if (!alloc().ensureBallast())
> > +                    return false;
> 
> I don't have expertise to review this really, even though it's such a small
> change. Why does this need to be in a loop? If it's needed here, why does
> not every use of new(alloc()) require a ensureBallast?

The jit's TempAllocator has an allocateInfallible() method that works by pre-allocating a 16KB ballast and then assuming that we don't allocate more than this without a call to a fallible allocate method or to ensureBallast().  This is asserted in debug builds.  The issue is that loops containing infallible allocations can allocate an unbounded amount of memory, so we need to call ensureBallast() once every loop iteration.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: test-module-eval-oom v2

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8695948 [details] [diff] [review]
test-module-eval-oom v2

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

::: js/src/jit/Ion.cpp
@@ +3116,5 @@
>                      cancelOffThread);
>  }
>  
>  bool
>  jit::IonScript::invalidate(JSContext* cx, bool resetUses, const char* reason)

Could you change this and jit::Invalidate to void if they are to be infallible now?

@@ +3125,2 @@
>      RecompileInfoVector list;
> +    MOZ_ALWAYS_TRUE(list.reserve(1));

Let's make this a MOZ_RELEASE_ASSERT. Not invalidating when we need if someone changes a data structure definition in the future would be bad.

@@ +3160,3 @@
>      RecompileInfoVector scripts;
>      MOZ_ASSERT(script->hasIonScript());
> +    MOZ_ALWAYS_TRUE(scripts.reserve(1));

Ditto on MOZ_RELEASE_ASSERT.

::: js/src/jit/IonBuilder.cpp
@@ +8153,5 @@
>  
> +    if (!loadStaticName(staticObject, barrier, types, property.maybeTypes()->definiteSlot())) {
> +        *psucceeded = false;
> +        return false;
> +    }

The stuff I suggested to be refactored starts earlier, at the "PropertyReadNeedsTypeBarrier" line in getStaticName. Is it incorrect to do that for getimport?

Comment 7

[Jon Coppeard (:jonco)]

Attachment: test-module-eval-oom v3

(In reply to Shu-yu Guo [:shu] from comment #6)
> The stuff I suggested to be refactored starts earlier, at the
> "PropertyReadNeedsTypeBarrier" line in getStaticName. Is it incorrect to do
> that for getimport?

Ok, I refactored this some more.  I couldn't move all the micro optimisations out of getStaticName() only the ones following the constantValue one since that depends on type information.  Is this what you had in mind?

I couldn't see how to factor out the calls to PropertyReadNeedsTypeBarrier() unfortunately.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8697048 [details] [diff] [review]
test-module-eval-oom v3

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

Thanks for the revisions. Sorry for the delay.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c575d46bfc2d

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c575d46bfc2d