985130 - OdinMonkey: omit over-recursion check in small leaf functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: omit-overflow-checks.patch

In small functions, the overhead of the MAsmJSCheckOverRecurrsed operation can take a noticeable percent of the time to run the function.

Attached is a sketch of a patch which implements something like this. It checks stack usage after register allocation, so it's safe from unbounded stack growth due to spilling, but I haven't looked into whether there are other hazards yet. But assuming something like this can be done easily, it may be worth the modest gain.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: omit-overflow-checks.patch

Update the patch to optimize regular LCheckOverRecursed in addition to  LAsmJSCheckOverRecursed.

Comment 2

[Dan Gohman [:sunfish]]

Comment on attachment 8403568 [details] [diff] [review]
omit-overflow-checks.patch

As far as I've been able to determine, checking the fixed stack frame after register allocation, and checking for any calls, should be sufficient.

This patch may be over-conservative, since we might be able to tolerate a small fixed amount of stack space, or calls to simple known library functions, but I'm keeping it simple for now.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8403568 [details] [diff] [review]
omit-overflow-checks.patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +8202,5 @@
>  bool
>  CodeGenerator::visitAsmJSCheckOverRecursed(LAsmJSCheckOverRecursed *lir)
>  {
> +    // If we don't push anything on the stack, skip the check.
> +    if (frameSize() == 0 && !gen->performsCall())

Could you factor this predicate out into one shared place?  Also, I'd bump the frameSize() limit to < 64, but that's just me ;)

Comment 4

[Dan Gohman [:sunfish]]

(In reply to Luke Wagner [:luke] from comment #3)
> Could you factor this predicate out into one shared place?  Also, I'd bump
> the frameSize() limit to < 64, but that's just me ;)

Done, and done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/109ea225a968

Comment 5

[Dan Gohman [:sunfish]]

Also, https://hg.mozilla.org/integration/mozilla-inbound/rev/bf165cddae07

since I conflated MInstruction::isCall() and LInstruction::isCall().

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/109ea225a968
https://hg.mozilla.org/mozilla-central/rev/bf165cddae07