Last Comment Bug 761460 - IonMonkey: Don't try to load Ion code for disabled JSScripts.
: IonMonkey: Don't try to load Ion code for disabled JSScripts.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-04 17:48 PDT by Sean Stangl [:sstangl]
Modified: 2012-06-05 17:16 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch. (4.31 KB, patch)
2012-06-04 17:48 PDT, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description Sean Stangl [:sstangl] 2012-06-04 17:48:12 PDT
Created attachment 630017 [details] [diff] [review]
Patch.

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%.
Comment 1 David Anderson [:dvander] 2012-06-04 19:43:52 PDT
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.
Comment 2 Hannes Verschore [:h4writer] 2012-06-05 05:00:32 PDT
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?
Comment 3 Sean Stangl [:sstangl] 2012-06-05 17:14:01 PDT
(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.
Comment 4 Sean Stangl [:sstangl] 2012-06-05 17:16:02 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/ff28f1696c5a

Note You need to log in before you can comment on or make changes to this bug.