Last Comment Bug 741111 - IonMonkey: slower on reduced SS crypto-aes benchmark
: IonMonkey: slower on reduced SS crypto-aes benchmark
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Hannes Verschore [:h4writer]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-31 09:41 PDT by Hannes Verschore [:h4writer]
Modified: 2012-04-04 18:40 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Inline constructing native calls (1.00 KB, patch)
2012-04-01 07:52 PDT, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Inline constructing native calls (2.98 KB, patch)
2012-04-03 18:14 PDT, Hannes Verschore [:h4writer]
sstangl: review+
Details | Diff | Splinter Review

Description Hannes Verschore [:h4writer] 2012-03-31 09:41:37 PDT
On awfy I saw we were still slower on crypto-aes. So I tried the reducer on that benchmark.
It returned the following testcase:

function MixColumns(s) {
    for (var c = 0; c < 4; c++) {
        var a = new Array(4);
    }
}
for (var i = 0; i < 40000; i++) {
    MixColumns();
}

JM: 0.063ms
V8: 0.130ms
IM: 0.200ms

The reason for the performance loss is the creation of the array. I assume that JM knows it isn't used and doesn't create it. IonMonkey does not, as removing the "var a = new Array(4);" makes IM faster then JM.

(Note: I'm not sure if fixing this will increase the score on the crypte-aes benchmark. As it is easy to introduce new slowdowns during reducing. But we are slower on this benchmark than JM, so definitly worth taking a look into.)
Comment 1 Hannes Verschore [:h4writer] 2012-04-01 07:52:47 PDT
Created attachment 611277 [details] [diff] [review]
Inline constructing native calls

So this is what I've found. When possible we inline native calls. This is done for "Array()", but not for "new Array()", because we don't do the optimization when call is done from "JSOP_NEW". Now I think that inlineNativeCall is also possible when constructing=true. AFAIK new Array() is currently the only native call in "inlineNativeCall" that would get inlined.

- jit-tests doesn't fail
- IM patched: 0.098ms
Comment 2 Hannes Verschore [:h4writer] 2012-04-01 08:13:38 PDT
Comment on attachment 611277 [details] [diff] [review]
Inline constructing native calls

Wow, that was totally wrong what I was saying. Ignore comment above
Comment 3 Hannes Verschore [:h4writer] 2012-04-01 14:31:49 PDT
Comment on attachment 611277 [details] [diff] [review]
Inline constructing native calls

I removed patch because I assumed it introduced a bug. This isn't the case as the bug is also present on tip (bug 741241). So patch is valid.

1.01x increase in SS
- 1.025x 3d-cube
- 1.12x  crypto-aes => So this testcase was indeed representative for total benchmark :D
Comment 4 David Anderson [:dvander] 2012-04-02 12:35:13 PDT
Comment on attachment 611277 [details] [diff] [review]
Inline constructing native calls

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

I think Sean should review this since he knows this code better than I now - though, I think we have to make sure in the call optimizer filter that we don't allow constructing for something like Math.round().
Comment 5 Sean Stangl [:sstangl] 2012-04-03 17:10:39 PDT
Comment on attachment 611277 [details] [diff] [review]
Inline constructing native calls

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

When natives are called with |new|, they typically expect to receive MagicValue(JS_CONSTRUCTING) as the |this| argument. Natives can inspect this value to determine whether they have been called with |new|. It looks like in the case of js_Array, it never consults that value, so no additional changes are necessary. In fact, we could just fail to pass |this| entirely. Just something to keep in mind. Passing it would involve significant refactoring work to inlineNativeCall(), and that's obnoxious, so it's OK if this patch ignores that.

0.98s isn't bad: visitNewArray() doesn't yet contain a fast-path for Array allocation, so once we get that, and after this patch, IM should be closer to JM perf.

::: js/src/ion/IonBuilder.cpp
@@ +2595,5 @@
>      // Acquire known call target if existent.
>      JSFunction *target = getSingleCallTarget(argc, pc);
>  
>      // Attempt to inline native and scripted functions.
> +    if (inliningEnabled() && target) {

This change is OK.

@@ +2600,1 @@
>          if (target->isNative() && inlineNativeCall(target, argc))

inlineNativeCall() must now take an additional |constructing| argument, and must currently return false in the case of |constructing && target->native() != js_Array|.

Since the set of native functions callable with |new| is significantly smaller than the set of native functions, this can probably be handled by checking at the top of inlineNativeCall(), before we even get into the functions.

Somewhat unrelatedly, inlineNativeCall() is a total mess. It badly needs to be broken up into multiple functions.
Comment 6 Hannes Verschore [:h4writer] 2012-04-03 18:14:46 PDT
Created attachment 612050 [details] [diff] [review]
Inline constructing native calls

Added the bool constructing to inlineNativeCall, like requested. Is indeed a bit cleaner then assuming the inline function will play nicely when new is thrown before them. Previous patch couldn't crash atm, because try-catch isn't supported, but when it does, it could get prone for errors.
Comment 7 Sean Stangl [:sstangl] 2012-04-04 14:05:41 PDT
Comment on attachment 612050 [details] [diff] [review]
Inline constructing native calls

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

Looks good. Do you have check-in access, or should I push for you?
Comment 8 Hannes Verschore [:h4writer] 2012-04-04 18:40:26 PDT
I have check-in access now, thanks. 4% on V8, 2% on SS.
http://hg.mozilla.org/projects/ionmonkey/rev/f9d6a7152d01

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