Closed
Bug 1001850
Opened 10 years ago
Closed 10 years ago
IonMonkey: Remove the intermediate native call when calling a bound function
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(1 file, 2 obsolete files)
10.05 KB,
patch
|
jandem
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
After a little bit of discussion in bug 1000780 we think it still might be good to have the bind() function done in ionmonkey (as extra) for the cases the selfhosted version is slower. Esp. since it isn't too complicated and we can get this already landed before the selfhosted version. Currently we when calling a bound function we do: ion -> native CallOrConstructBoundFunction -> ion Which is suboptimal. So we want to be able to do: part 1) Call ion out of ion and remove the native part 2) Inline the ionmonkey function I'm confident enough to try to get part 1 into FF31 already. Part 2 isn't that much difficult, but I probably won't have enough time to create this properly before Monday. So I'm placing my bets to get part 1 landed.
Assignee | ||
Comment 1•10 years ago
|
||
In order to get a quick round trip time, I'm already asking for review. This is almost done. I only need to create tests and test the logic for constructing (see TODO). But this is the meat. Note: I created "makeCallReplacingNative". During the workweek I finally understood that our adjustments we did for "baselineBailouts" for JSOP_FUNAPPLY/JSOP_FUNCALL/... are unnecessary! This is the proper way :D. So eventually I will adjust those to use this mechanism too, removing the ugly code in baselineBailouts \0/.
Assignee: nobody → hv1989
Attachment #8413206 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•10 years ago
|
||
This should be it. Gives same performance numbers as bug 1000780. Try run: https://tbpl.mozilla.org/?tree=Try&rev=a6cfdb09f0df
Attachment #8413206 -
Attachment is obsolete: true
Attachment #8413206 -
Flags: review?(jdemooij)
Attachment #8413460 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•10 years ago
|
||
Just realizing "makeCallReplacingNative" is not needed. We only need to fake it for inlined frames!
Attachment #8413460 -
Attachment is obsolete: true
Attachment #8413460 -
Flags: review?(jdemooij)
Attachment #8413483 -
Flags: review?(jdemooij)
Comment 4•10 years ago
|
||
Comment on attachment 8413483 [details] [diff] [review] bug1001850-inline-bind Review of attachment 8413483 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, r=me with comments below addressed. ::: js/src/jit/IonBuilder.h @@ +717,5 @@ > InliningStatus inlineBailout(CallInfo &callInfo); > InliningStatus inlineAssertFloat32(CallInfo &callInfo); > > + // Bind function. > + InliningStatus inlineBind(CallInfo &callInfo, JSFunction *target); This doesn't inline .bind but the result of it, so inlineBoundFunction? Also change the comment to "Bound function." ::: js/src/jit/MCallOptimize.cpp @@ +193,5 @@ > return inlineBailout(callInfo); > if (native == testingFunc_assertFloat32) > return inlineAssertFloat32(callInfo); > > + // Bind function Nit: // Bound function. @@ +1920,5 @@ > + return InliningStatus_NotInlined; > + > + for (size_t i = 0; i < target->getBoundFunctionArgumentCount(); i++) { > + const Value val = target->getBoundFunctionArgument(i); > + if (val.isObject() && !val.toObject().isTenured()) isTenured is only defined in debug builds so use !gc::isInsideNursery(rt, val.toObject()) @@ +1925,5 @@ > + return InliningStatus_NotInlined; > + } > + > + const Value thisVal = target->getBoundFunctionThis(); > + if (thisVal.isObject() && !thisVal.toObject().isTenured()) Same here. @@ +1934,5 @@ > + CallInfo callInfo(alloc(), nativeCallInfo.constructing()); > + callInfo.setFun(constant(ObjectValue(*scriptedTarget))); > + callInfo.setThis(constant(target->getBoundFunctionThis())); > + > + size_t argc = target->getBoundFunctionArgumentCount() + nativeCallInfo.argc(); To match CallOrConstructBoundFunction, we should also check: if (argc > ARGS_LENGTH_MAX) return InliningStatus_NotInlined;
Attachment #8413483 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Thanks for the quick review! https://hg.mozilla.org/integration/mozilla-inbound/rev/53649d31c8b4 (try looks good: https://tbpl.mozilla.org/?tree=Try&rev=a5a7f08387e5)
https://hg.mozilla.org/mozilla-central/rev/53649d31c8b4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8413483 [details] [diff] [review] bug1001850-inline-bind [Approval Request Comment] Bug caused by (feature/regressing bug #): Feature: Improving speed of bounded function (.bind) three-fold User impact if declined: Not improved performance on a bounded function, which is used a lot in JS. Testing completed (on m-c, etc.): m-c: 1 day already Risk to taking this patch (and alternatives if risky): Risk is low. This is only part of the full scope, that was easiest and shouldn't cause regressions, but still gives a nice boost. This just missed the merge by a few hours. String or IDL/UUID changes made by this patch: /
Attachment #8413483 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Summary: IonMonkey: inline a bound function → IonMonkey: Remove the intermediate native call when calling a bound function
Updated•10 years ago
|
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
Updated•10 years ago
|
Attachment #8413483 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 9•10 years ago
|
||
Great to see this was done already, thanks! (In reply to Hannes Verschore [:h4writer] from comment #0) > I'm confident enough to try to get part 1 into FF31 already. Part 2 isn't > that much difficult, but I probably won't have enough time to create this > properly before Monday. So I'm placing my bets to get part 1 landed. What's part 2? Bug 1000780? (In reply to Hannes Verschore [:h4writer] from comment #1) > Note: I created "makeCallReplacingNative". During the workweek I finally > understood that our adjustments we did for "baselineBailouts" for > JSOP_FUNAPPLY/JSOP_FUNCALL/... are unnecessary! This is the proper way :D. > So eventually I will adjust those to use this mechanism too, removing the > ugly code in baselineBailouts \0/. Is there a bug on file for this already?
Flags: needinfo?(hv1989)
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Florian Bender from comment #9) > What's part 2? Bug 1000780? > Is there a bug on file for this already? I tried already to do part 2, but it seems a bit more complicated than I described in comment #1. It still isn't that easy. So I could do it like JSOP_FUNAPPLY etc is done. But that is error prone and we now do this already for FUNAPPLY/FUNCALL/getters/setters. So we should probably abstract this, but that hasn't happen yet. It is on my todo list to look into this.
Flags: needinfo?(hv1989)
Comment 11•10 years ago
|
||
Hannes, does this need any testing from QA before we release? Thanks!
Flags: needinfo?(hv1989)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Liz Henry :lizzard from comment #11) > Hannes, does this need any testing from QA before we release? Thanks! I don't have a lot of experience with QA. But this is a regular feature that might boost performance of calling a function that has been bound (with .bind) which is used a lot. I think QA jumped on this, because it has been uplifted, right? Now I think this doesn't need extra or special care. It should strictly improve performance in a very managed and understood way for some specific usages. It won't regress other things. Part 2 is not intended for uplift and might even not land in this cycle, given it is harder than thought. So there is also no extra QA needed for that.
Flags: needinfo?(hv1989)
Comment 13•10 years ago
|
||
Hannes, thank you! I think I understand. Are there any specific urls that it would be good for us to look at?
Flags: needinfo?(hv1989)
Comment 14•10 years ago
|
||
It also looks like there is already a test built in so I'll flag it accordingly :)
Flags: in-testsuite?
Assignee | ||
Comment 15•10 years ago
|
||
No need for QA on this.
Flags: needinfo?(hv1989)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•