Closed Bug 941028 Opened 6 years ago Closed 6 years ago

IonMonkey: Don't inline in big functions

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Took some time to look into the mandreel regression.

The lost time is in compiling function _ZN13BenchmarkDemo11initPhysicsEv (:11347). Before the regression we disabled inlining tlsf_malloc (:106544). This is accidentally due to this rule:

> // Callee must be hot relative to the caller.
> if (targetScript->getUseCount() * js_IonOptions.inlineUseCountRatio < callerUses)

targetScript->getUsecount() == 55
js_IonOptions.inlineUsecountRatio = 128
callerUses is maximum 1000 normally. Since we start running in ionmonkey as soon usecount is 1000. So 55*128 > 1000 and we should inline that function.

But if compilation takes some time or have a lot of bailouts, we will still increase the usecount in baseline. As a result in this case callerUses gets 30000. And we decided to not inline.

So before the regression we weren't inlining it. Now after the regression our compiling time is less or we are less in baseline (what is good!). So callerUses is lower and we decide to inline that function. We inline ":106544" 150 times in ":11347". ":106544" has a size of 528. There we inline ":3449", size 25, 6 times. So (528 + 150)*150 = 101700. So that increases our size to 101700 and ":11347" is already 53000. So we need to compile something that is 15000. And size the compilation time is n^4 to our size. This is really bad!!

So eventually we want better heuristics to decide when to inline. And actually we want some global algorithms. So that we can view we are inlining a function 150 times and disable that.

For now that is not possible, so I did the next best thing. I added an heuristic to disable inlining when function is very large. My thought process is that in big functions the execution time is not limited by call overhead, but by the logic in the function. The larger a function becomes, the more functions it can contain. Resulting in more inlining and creating bigger total compilation. And we want to disable that.

This improves our score on octane1.0 from 23200 to 24700. Similar on octane2.0.
We also hit this case on Typescript, but it doesn't help much. Lowering the limit from 10000 to 1000 will increase typescript from 10000 to 12000. So we are also to aggressively inlining there. Doesn't hit SS/kraken. Does hit zlib (with --no-asmsj) and decreases time from 66563 to 17292. (Though there are some strange things, that I want awfy to confirm).

I took a rather high limit, so we don't accidental hit this for normal scripts. Also because we might want to take this patch upstream. (after verifying results on awfy).

(in the midterm I want to remove/alter that bogus rule above. It doesn't make sense with the lowered usecount limit of 1000 . Except to create strange cases like this.)
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → hv1989
Attachment #8335328 - Flags: review?(jdemooij)
Comment on attachment 8335328 [details] [diff] [review]
Patch

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

Thanks for looking into this regression. I think we still want to allow inlining small functions, like this one in Octane-Mandreel that's very hot:

function uint(value) {
    if (value >= 0) return value;
    return 4294967296 + value;
}

Compiling these should be pretty fast. Could we tweak the inlineUsecountRatio or the max callee bytecode length for large functions?

(In reply to Hannes Verschore [:h4writer] from comment #0)
> Does hit
> zlib (with --no-asmsj) and decreases time from 66563 to 17292. (Though there
> are some strange things, that I want awfy to confirm).

We should find out what's going on here.. Maybe also a small function that's very hot?
Attachment #8335328 - Flags: review?(jdemooij)
(I think the inlining heuristics already treat small functions differently. What if we only allowed these callees to be inlined?)
This only applies the threshold for large targets.

(Note: about TypeScript, I think you misunderstood. Lowering this threshold will increase the score a lot. I assume it is also inlining way to much, resulting in too large compilation times. So this has nothing to do with a small function)
Attachment #8336146 - Flags: review?(jdemooij)
Attachment #8335328 - Attachment is obsolete: true
(In reply to Hannes Verschore [:h4writer] from comment #4)
> (Note: about TypeScript, I think you misunderstood. Lowering this threshold
> will increase the score a lot. I assume it is also inlining way to much,
> resulting in too large compilation times. So this has nothing to do with a
> small function)

hmm I read your comment too fast. You were talking about zlib.
Comment on attachment 8336146 [details] [diff] [review]
bug941028-dont-inline-big

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

Thanks! Does this change our Mandreel/zlib score in any way compared to your previous patch? Please make sure we don't regress zlib (a lot) with --no-asmjs, r=me with that.

::: js/src/jit/Ion.h
@@ +203,5 @@
>      // Default: 1
>      uint32_t usesBeforeCompilePar;
>  
> +    // The maximum bytecode length the caller may have,
> +    // before we stop inlining any functions in that caller.

Nit: s/any/large
Attachment #8336146 - Flags: review?(jdemooij) → review+
Interesting enough this doesn't change the score on awfy on no-asmjs-zlib, octane-mandreel. It does still help on my computer a lot. So I'll keep it in.

(I assume that the bogus heuristic still blocks the inlining there)
Blocks: 942105
https://hg.mozilla.org/mozilla-central/rev/a792845bcee7
https://hg.mozilla.org/mozilla-central/rev/123f9dbe5d95
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.