As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
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.
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: general
: Jason Orendorff [:jorendorff]
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image Sean Stangl [:sstangl] 2012-06-04 17:48:12 PDT
Created attachment 630017 [details] [diff] [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%.
Comment 1 User image David Anderson [:dvander] 2012-06-04 19:43:52 PDT
Comment on attachment 630017 [details] [diff] [review]

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 User image 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 User image 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 User image Sean Stangl [:sstangl] 2012-06-05 17:16:02 PDT

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