823884 - IonMonkey: Abort during inlining will disable top script

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

When inlining a function in IonBuilder and the inlined function aborts we will disable the script that's inlining the function.

This should get adjusted to disable inlining that function.

I would prefer not disabling the inlined function. As since bug 813784 we can have an abort that only happens when the function is inlined. Therefore only disable script when the function is not inlined.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Don't disable top script

When an abort happens in an inlined function, that function will be set to uninlineable and the function will get recompiled again.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 696295 [details] [diff] [review]
Don't disable top script

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

::: js/src/ion/Ion.cpp
@@ +1148,5 @@
>  #endif
>  
>      LifoAlloc *alloc = cx->new_<LifoAlloc>(BUILDER_LIFO_ALLOC_PRIMARY_CHUNK_SIZE);
>      if (!alloc)
> +        return AbortReason_Unknown;

nit: Use AbortReason_Alloc for all AbortReason_Unknown of this function.

@@ +1254,5 @@
> +
> +    // When compiling failed due to inlined function, retry the compilation.
> +    // The function has been disabled and won't cause repeated compilations.
> +    while (reason == AbortReason_Inlining) {
> +        reason = IonCompile(cx, script, fun, osrPc, constructing, compileContext);

see comment below.

@@ +1384,5 @@
>      SequentialCompileContext compileContext;
> +
> +    AbortReason reason = IonCompile(cx, script, fun, osrPc, constructing, compileContext);
> +    while (reason == AbortReason_Inlining)
> +        reason = IonCompile(cx, script, fun, osrPc, constructing, compileContext);

We should never loop if a compilation is failing.  Otherwise it might be too complex to determine if the compilation terminates or not, especially with memory alloc issues.

Then, if for any reason we never succeed the compilation, not looping at the compilation will still bring us closer to the termination of the program, where a compilation loop will hang forever.

::: js/src/ion/Ion.h
@@ +213,5 @@
>  
> +enum AbortReason {
> +    AbortReason_Inlining,
> +    AbortReason_Unknown,
> +    AbortReason_NoAbort

Great, would be good to add AbortReason_Alloc, and avoid forbidding the compilation if we abort due to an allocation issue.  And AbortReason_Unknown should be renamed as AbortReason_Disable.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Don't disable top script

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 696329 [details] [diff] [review]
Don't disable top script

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

Sounds good.

::: js/src/ion/Ion.cpp
@@ +1171,5 @@
>  
>      types::AutoEnterTypeInference enter(cx, true);
>      TypeInferenceOracle oracle;
>  
> +    //@nbp: I don't think this is always an alloc thing...

nit: remove before pushing. But a non-dedicated comment might still be worth it ;)

Comment 5

[Hannes Verschore [:h4writer]]

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

Comment 6

[Josh Matthews [:jdm]]

Backed out as part of a set of patches suspected of causing permaorange Android 2.2 opt m2 and m3:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d2552e3d3ed6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e497c2e40dc6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22e38c3125e9

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: M1/M2 fixes

After the builder succeeded we could still fail to make the full IM compilation. Something that never happened on non-android. Upon fail we still took the "abortReason" from the builder, that happily reported NoAbort, resulting in trying to enter non-existing code...

I'm today when both review and tbpl reports green.
https://tbpl.mozilla.org/?tree=Try&rev=bc60f7d9e8d5

Else I can only push this next week (as I'm on holiday till Monday)

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #7)
> I'm *pushing* today when both review and tbpl reports green.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 698650 [details] [diff] [review]
M1/M2 fixes

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

Fix back end abort reason and r=me

::: js/src/ion/Ion.cpp
@@ +1240,5 @@
>  
>      CodeGenerator *codegen = CompileBackEnd(builder);
>      if (!codegen) {
>          IonSpew(IonSpew_Abort, "Failed during back-end compilation.");
> +        return AbortReason_Disable;

If we have any reason to disable a compilation, we are supposed to do it in IonBuilder. I don't think any phase is failing the compilation if it cannot be applied, unless there is an allocation issue.

Replace this by:
    return AbortReason_Alloc

Comment 10

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #9)
> If we have any reason to disable a compilation, we are supposed to do it in
> IonBuilder. I don't think any phase is failing the compilation if it cannot
> be applied, unless there is an allocation issue.
> 
> Replace this by:
>     return AbortReason_Alloc

This isn't caused by an allocation issue. Opened bug 827421 for this. I'll push this now with AbortReason_Disable, to not alter behaviour and compiling that script on every run. After 827421 gets fixed I'll adjust it to AbortReason_Alloc.

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dcb5963e1ce

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4dcb5963e1ce