Closed Bug 823884 Opened 11 years ago Closed 11 years ago

IonMonkey: Abort during inlining will disable top script

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 768288
Attached patch Don't disable top script (obsolete) — Splinter Review
When an abort happens in an inlined function, that function will be set to uninlineable and the function will get recompiled again.
Assignee: general → hv1989
Attachment #696295 - Flags: review?(nicolas.b.pierron)
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.
Attachment #696295 - Flags: review?(nicolas.b.pierron)
Attachment #696295 - Attachment is obsolete: true
Attachment #696329 - Flags: review?(nicolas.b.pierron)
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 ;)
Attachment #696329 - Flags: review?(nicolas.b.pierron) → review+
Attached patch M1/M2 fixesSplinter Review
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)
Attachment #698650 - Flags: review?(nicolas.b.pierron)
(In reply to Hannes Verschore [:h4writer] from comment #7)
> I'm *pushing* today when both review and tbpl reports green.
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
Attachment #698650 - Flags: review?(nicolas.b.pierron) → review+
Depends on: 827421
(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.
https://hg.mozilla.org/mozilla-central/rev/4dcb5963e1ce
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: