Closed
Bug 741111
Opened 12 years ago
Closed 12 years ago
IonMonkey: slower on reduced SS crypto-aes benchmark
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: h4writer, Assigned: h4writer)
Details
Attachments
(1 file, 1 obsolete file)
2.98 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
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)
Assignee | ||
Comment 3•12 years ago
|
||
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 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
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.
Description
•