Closed Bug 741111 Opened 12 years ago Closed 12 years ago

IonMonkey: slower on reduced SS crypto-aes benchmark

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(1 file, 1 obsolete file)

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.)
Attached patch Inline constructing native calls (obsolete) — Splinter Review
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
Attachment #611277 - Flags: review?(dvander)
Comment on attachment 611277 [details] [diff] [review]
Inline constructing native calls

Wow, that was totally wrong what I was saying. Ignore comment above
Attachment #611277 - Attachment is obsolete: true
Attachment #611277 - Flags: review?(dvander)
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
Attachment #611277 - Attachment is obsolete: false
Attachment #611277 - Flags: review?(dvander)
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().
Attachment #611277 - Flags: review?(dvander) → review?(sstangl)
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.
Attachment #611277 - Flags: review?(sstangl)
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.
Assignee: general → hv1989
Attachment #611277 - Attachment is obsolete: true
Attachment #612050 - Flags: review?(sstangl)
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?
Attachment #612050 - Flags: review?(sstangl) → review+
I have check-in access now, thanks. 4% on V8, 2% on SS.
http://hg.mozilla.org/projects/ionmonkey/rev/f9d6a7152d01
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.