Closed Bug 761460 Opened 12 years ago Closed 12 years ago

IonMonkey: Don't try to load Ion code for disabled JSScripts.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Unassigned)

Details

Attachments

(1 file)

Attached patch Patch.Splinter Review
kraken-1.1's audio-beat-detection is about twice as slow as the competition. I measured that slightly more than half (484 of 780ms) of our total execution time is spent in the function at audio-beat-detection-data:2147, which is too long to be compiled by Ion. However, we always enter it from Ion.

This first simple patch doesn't go through checking whether Ion code has been compiled for scripts that have already been disabled. Brings test from 780 to 750ms, about 4%.
Attachment #630017 - Flags: review?(dvander)
Comment on attachment 630017 [details] [diff] [review]
Patch.

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

::: js/src/ion/CodeGenerator.cpp
@@ +561,5 @@
> +            Label notPrimitive;
> +            masm.branchTestPrimitive(Assembler::NotEqual, JSReturnOperand, &notPrimitive);
> +            masm.loadValue(Address(StackPointer, unusedStack), JSReturnOperand);
> +            masm.bind(&notPrimitive);
> +        }

You might be able to share this too (it's repeated) but I don't know if its worth it.
Attachment #630017 - Flags: review?(dvander) → review+
Nice improvement. 

I was wondering if it would make sense to keep track of scripts that get disabled?
When script A gets compiled and script B has not compiled yet, the extra code (causing the slowdown will get added). Eventualy when Ion tries to compile B, but doesn't succeed (Ion decides it is to long or try is used ...) the extra code in script A will remain. Causing the slowdown ...

What about recompiling script A when script B is set to ION_DISABLED_SCRIPT?
(In reply to Hannes Verschore from comment #2)
> What about recompiling script A when script B is set to ION_DISABLED_SCRIPT?

The overhead of running the checking instructions is likely less than the cost of invalidating and recompiling script A. We tend to re-JIT functions pretty frequently anyway (even with the patch preserving code on GC), so callsites will pretty rapidly converge on the right configuration without additional help.
http://hg.mozilla.org/projects/ionmonkey/rev/ff28f1696c5a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.