844246 - BaselineCompiler: Run inference in time to ensure that Ion can inline calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Right now, Ion will only inline a scripted call if TI has run on the called script; analyzing types in the script during IonBuilder can change type sets which have previously been used by the builder, so IonBuilder (well, TypeInferenceOracle::canEnterInlinedFunction) simply checks and skips callees that TI hasn't analyzed.

This works fine with JM+TI as compiling with JM+TI requires running inference on a script, but plays havoc with Baseline --- if the caller has a higher use count than the callee (which will very often be the case) then the callee will not be inlined unless some future type change invalidates the caller but not the callee.

Baseline needs a mechanism to analyze the types in callees at the start of IonBuilder, if they are hot enough that they could be inlined.  This can go away after bug 804676.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Do ensureRanInference on callee tree before compiling with Ion

I ran into this when looking at deltablue.

The following patches recursively does ensureRanInference on callees snooped from baseline IC caches before entering into Ion compilation.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: alternative

This is a slightly different patch that does a couple things differently:

- No state is kept in the baseline structures, so that scripts can be scanned multiple times during different compilations (this is good to do as the callees' use counts can change and thus the behavior of scanning the caller).

- Uses TI state, so that types can be analyzed in 'this' values for foo.call and foo.apply invocations, allowing those to be inlined by Ion.

This improves my v8bench score by 400 points or so and also improves SS a few ms.

Comment 3

[Kannan Vijayan [:djvj]]

Comment on attachment 717330 [details] [diff] [review]
alternative

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

Looks good.

::: js/src/ion/TypeOracle.cpp
@@ +96,5 @@
> +        if (JSObject *singleton = calleeTypes->getSingleObject(i)) {
> +            if (singleton->isFunction() && singleton->toFunction()->hasScript())
> +                script = singleton->toFunction()->nonLazyScript();
> +            else
> +                continue;

Style nit: The code structure is a bit hard to read as the first branch "falls through", but the continue comes after the fallthrough, but is on the other branch so the fallthrough doesn't actually hit it.

Would be clearer as:

if (!singleton->isFunction())
    continue;
if (!singleton->toFunction()->hasScript())
    continue;
script = singleton->toFunction()->nonLazyScript();

@@ +102,5 @@
> +            JSFunction *fun = type->interpretedFunction;
> +            if (fun && fun->hasScript())
> +                script = fun->nonLazyScript();
> +            else
> +                continue;

See comment above.

if (!fun || !fun->hasScript())
    continue;
script = fun->nonLazyScript();

@@ +104,5 @@
> +                script = fun->nonLazyScript();
> +            else
> +                continue;
> +        } else {
> +            continue;

Is it possible for an object to be something other than a Singleton or a TypeObject?  Should there be a JS_NOT_REACHED here?

Comment 4

[Brian Hackett [Laid off!]]

TypeSets can return NULL for both getSingleObject and getTypeObject, as for larger sets they are iterating over a hashtable.

https://hg.mozilla.org/projects/ionmonkey/rev/8245e95ef08f