Closed Bug 1136837 Opened 5 years ago Closed 5 years ago

Fix some inlining issues

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

I just noticed a few issues on Octane-TypeScript:

(1) We're inlining functions even if |this| or one of the arguments has an empty resultTypeSet. We probably shouldn't do that because it means the call will never be reached (without recompiling the script). Inlining in this case *could* help alias analysis, but I really doubt it's worth the extra compile time.

(2) When we have:

  if (o && o.foo())

Inside foo, we may not know that |this| is an object (it could have an object-or-null/undefined TypeSet). If o is null/undefined, o.foo would throw an exception though so |this| must be an object. Currently we add MComputeThis, which has no resultTypeSet, so using |this| is much slower than necessary.
This fixes the first problem in comment 0. This keeps us from inlining a lot of scripts on TypeScript (though it's possible they'd hit other aborts later) and we win a few hundred points locally (but noisy). The difference is a bit bigger and more reliable with --no-threads, which makes sense.

The inlining abort also hits on other benchmarks like deltablue, pdfjs, box2d and mandreel, but I see no difference in our score there.
Attachment #8569699 - Flags: review?(hv1989)
Comment on attachment 8569699 [details] [diff] [review]
Part 1 - Don't inline if argument types are incomplete

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

Nice.

::: js/public/TrackedOptimizationInfo.h
@@ +203,5 @@
>        "can't inline: not hot enough")                                   \
>      _(CantInlineNotInDispatch,                                          \
>        "can't inline: not in dispatch table")                            \
> +    _(CantInlineUnreachable,                                            \
> +      "can't inline: unreachable due to incomplete this/argument types")\

s/argument/arguments

::: js/src/jit/IonBuilder.cpp
@@ +4691,5 @@
> +                trackOptimizationOutcome(TrackedOutcome::CantInlineUnreachable);
> +                return InliningDecision_DontInline;
> +            }
> +        }
> +    }

Can you move this into "canInlineTarget"? I think non heuristic callee checks should go in there.
Attachment #8569699 - Flags: review?(hv1989) → review+
This fixes the second problem and another minor related issue that came up on TypeScript.
Attachment #8569729 - Flags: review?(hv1989)
Thanks for the quick review.

(In reply to Hannes Verschore [:h4writer] from comment #2)
> > +    _(CantInlineUnreachable,                                            \
> > +      "can't inline: unreachable due to incomplete this/argument types")\
> 
> s/argument/arguments

"incomplete arguments types" is not correct English, AFAIK, but I can rephrase it as "incomplete types for this/arguments".

> Can you move this into "canInlineTarget"?

Sure.
Attachment #8569729 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/1fb224ec0020
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1139368
You need to log in before you can comment on or make changes to this bug.