1062648 - Improve use of callee use count in inlining heuristics

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently, one of the heuristics used to decide whether to inline scripts during IonBuilder is based on the use count of the callee, the intention being to avoid inlining scripts which are cold and haven't accumulated much type information or execute that much.  This is problematic because after a script is first Ion compiled, its use count is reset whenever its type information changes.  So in such cases this heuristic ends up testing the number of times the script has been called since the last time its type information last changed, which isn't very meaningful.  Per bug 1041688 comment 41 this is now affecting our octane-richards score.  The attached patch changes this heuristic so we don't take into account the use count of callee scripts which have ever been inlined or Ion compiled.  On my machine this improves our richards score by a few thousand points, and doesn't seem to have an effect on the other octane benchmarks.

Comment 1

[Hannes Verschore [:h4writer]]

Hmm, this looked a good approach on first sight, but I have some doubts now.

The useCount is actually just a measurement to know if it has run long enough to have good type information. We used to reset the useCount when TI information was removed. Now since we retain TI information accross GC, a better fix would be to not reset the usecount on GC. As a result we wouldn't need this "has ever had IonMonkey code".

BUT type information is more than TI information alone. We also look at the baseline caches to for type information. And these are still flushed on GC. So thinking about that, it means useCount is actually correct in still reseting the useCount ...
AND as a result I'm also inclined to think that this patch is a bit hackish, since it ignores the fact that we could have worse type information by not having baseline cache information.

So that is my current thoughts, I hope you understand my concerns? Now I'm open for discussion thought. Also the reason why I added Jan.

Also note I have "bug 911738", which might be a better approach. This will add a MIR that increases the usecount of not inlined scripts and recompiles a script when a callee becomes hot enough to inline. Using that we also get the few thousand points increase mentioned here. (Though it doesn't fix the bimodality). I think it might be a better approach and if you agree with it, I'll put my full concentration on it to fix the bimodality and land this instead? (To fix the regression).

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Hannes Verschore [:h4writer] from comment #1)
> The useCount is actually just a measurement to know if it has run long
> enough to have good type information. We used to reset the useCount when TI
> information was removed. Now since we retain TI information accross GC, a
> better fix would be to not reset the usecount on GC. As a result we wouldn't
> need this "has ever had IonMonkey code".

See also Bug 825268.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Hannes Verschore [:h4writer] from comment #1)
> Hmm, this looked a good approach on first sight, but I have some doubts now.
> 
> The useCount is actually just a measurement to know if it has run long
> enough to have good type information. We used to reset the useCount when TI
> information was removed. Now since we retain TI information accross GC, a
> better fix would be to not reset the usecount on GC. As a result we wouldn't
> need this "has ever had IonMonkey code".
> 
> BUT type information is more than TI information alone. We also look at the
> baseline caches to for type information. And these are still flushed on GC.
> So thinking about that, it means useCount is actually correct in still
> reseting the useCount ...
> AND as a result I'm also inclined to think that this patch is a bit hackish,
> since it ignores the fact that we could have worse type information by not
> having baseline cache information.
> 
> So that is my current thoughts, I hope you understand my concerns? Now I'm
> open for discussion thought. Also the reason why I added Jan.

This analysis ignores the issue of the use count being reset when type information changes, which is the main thing this patch is trying to help with.

During GC, behavior will depend on whether we are sweeping or preserving jitcode.

- If we're sweeping jitcode, the baseline scripts will be swept and the ION_COMPILED bit wiped out, so no scripts will be treated as if they were ever Ion compiled.  This will give the same behavior as what we have currently.

- If we're preserving jitcode, the baseline scripts will all be retained along with the ION_COMPILED bits.  However, their caches will be retained as well --- the GC calls PurgeJITCaches, but all this ends up doing is purging caches from IonScripts.

There is an exception to the above behavior in that if we are sweeping jitcode and there are baseline scripts on the stack, those baseline scripts will be retained but have their ICs purged.  For completeness we could clear the ION_COMPILED bits when we do this, but this scenario should be extremely rare, especially in benchmarks, since if there are any ion frames on the stack as well we will preserve jitcode during the GC.

> Also note I have "bug 911738", which might be a better approach. This will
> add a MIR that increases the usecount of not inlined scripts and recompiles
> a script when a callee becomes hot enough to inline. Using that we also get
> the few thousand points increase mentioned here. (Though it doesn't fix the
> bimodality). I think it might be a better approach and if you agree with it,
> I'll put my full concentration on it to fix the bimodality and land this
> instead? (To fix the regression).

This would be good but I think both patches should land.  Doing just bug 911738 would paper over this problem, as we would still invalidate and recompile the calling script more than is necessary, on account of using the script's use count in an inappropriate way.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch v2

Clear the ION_COMPILED bit when purging a baseline script's ICs.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8484339 [details] [diff] [review]
patch v2

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

Like discussed on IRC, it is more clear to rename ION_COMPILED to ION_COMPILED_OR_INLINED. Since this bit also represents if it was inlined ever.

::: js/src/jit/BaselineJIT.h
@@ +153,5 @@
>          // Debugger hooks and compiles toggled calls for traps.
> +        DEBUG_MODE = 1 << 3,
> +
> +        // Flag set if this script has ever been Ion compiled, either directly
> +        // or inlined into another script.

Can you add a comment that this is cleared when "types" are cleared?

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/697df33eae47

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/697df33eae47