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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Part 1: Remove native call (obsolete) — Splinter Review
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)
Attached patch bug1001850-inline-bind (obsolete) — Splinter Review
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)
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 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+
https://hg.mozilla.org/mozilla-central/rev/53649d31c8b4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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?
Summary: IonMonkey: inline a bound function → IonMonkey: Remove the intermediate native call when calling a bound function
Attachment #8413483 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
(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)
Hannes, does this need any testing from QA before we release? Thanks!
Flags: needinfo?(hv1989)
(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)
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)
It also looks like there is already a test built in so I'll flag it accordingly  :)
Flags: in-testsuite?
No need for QA on this.
Flags: needinfo?(hv1989)
Flags: in-testsuite?
Flags: in-testsuite+
Depends on: 1148883
You need to log in before you can comment on or make changes to this bug.