Inline Boolean when called as a function

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 months ago
I saw this in a Speedometer/React profile and it should be easy enough to inline in MCallOptimize using IonBuilder::convertToBoolean(...).
(In reply to André Bargull from comment #0)
> I saw this in a Speedometer/React profile and it should be easy enough to
> inline in MCallOptimize using IonBuilder::convertToBoolean(...).

Can you provide more information, with ideally a micro benchmark which highlight the issue?
(Assignee)

Comment 2

6 months ago
    var q = 0;
    var inputs = [true, false, 0, 1, "", "awd", null, {}];
    var t = Date.now();
    for (var i = 0; i < 10000000; ++i) {
        if (Boolean(inputs[i & 0x7])) q++;
    }
    print(Date.now() - t, q);

Would improve from 125ms to ~30ms (same number as V8, which also optimizes calls to the Boolean constructor -> https://github.com/v8/v8/blob/a234136074366a8ab8d1c703bad1949ec5a1f033/src/compiler/js-call-reducer.cc#L67-L81).

FWIW in Speedometer, this React function seems to cause most Boolean calls: https://github.com/facebook/react/blob/0.14-stable/src/renderers/shared/reconciler/ReactCompositeComponent.js#L689
(Assignee)

Comment 3

6 months ago
Created attachment 8874444 [details] [diff] [review]
bug1370208.patch

The patch used for comment #2.

Do you think this makes sense to optimize?
Attachment #8874444 - Flags: feedback?(nicolas.b.pierron)
Comment on attachment 8874444 [details] [diff] [review]
bug1370208.patch

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

::: js/src/jit/MCallOptimize.cpp
@@ +904,5 @@
>  
>  IonBuilder::InliningResult
> +IonBuilder::inlineBoolean(CallInfo& callInfo)
> +{
> +    if (callInfo.argc() != 1 || callInfo.constructing()) {

nit/follow-up: It seems that the 0 argument case, or more than one case should be easy to implement as well.

http://searchfox.org/mozilla-central/source/js/src/jsbool.cpp#116
Attachment #8874444 - Flags: feedback?(nicolas.b.pierron) → review+
(Assignee)

Comment 5

6 months ago
Created attachment 8874744 [details] [diff] [review]
bug1370208.patch

Relaxed arguments check to allow only number of arguments per comment #4. Carrying r+.
Attachment #8874444 - Attachment is obsolete: true
Attachment #8874744 - Flags: review+
(Assignee)

Comment 6

6 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2f4711c9d7c5e898216a8a1a893dded9170aa5
Keywords: checkin-needed

Comment 7

6 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f302f3481aaa
Inline Boolean constructor when called as a function. r=nbp
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f302f3481aaa
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.