Closed Bug 985130 Opened 6 years ago Closed 6 years ago

OdinMonkey: omit over-recursion check in small leaf functions

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: sunfish, Assigned: sunfish)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch omit-overflow-checks.patch (obsolete) — Splinter Review
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.
Update the patch to optimize regular LCheckOverRecursed in addition to  LAsmJSCheckOverRecursed.
Assignee: nobody → sunfish
Attachment #8393110 - Attachment is obsolete: true
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.
Attachment #8403568 - Flags: review?(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 ;)
Attachment #8403568 - Flags: review?(luke) → review+
(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
Also, https://hg.mozilla.org/integration/mozilla-inbound/rev/bf165cddae07

since I conflated MInstruction::isCall() and LInstruction::isCall().
https://hg.mozilla.org/mozilla-central/rev/109ea225a968
https://hg.mozilla.org/mozilla-central/rev/bf165cddae07
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.