Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More specific if conditions lead to ~10% faster render. #610

Merged
merged 4 commits into from
Apr 2, 2017

Conversation

asolove
Copy link
Contributor

@asolove asolove commented Mar 26, 2017

I'm not sure if this is stuff you already found in your secret v8 work. But here goes...

if(variable) checks are super slow because the whole truthy/falsy thing is complicated. You mentioned innerDiffNode was the hottest thing on your benchmarks, and I was reading through it to try to explain it, and noticed some flagrant usage of vague if(variable) tests. By being very explicit that we only want number comparison to 0, they get faster. On my computer, running the benchmarks test, I see everything 1-5% faster after this change. (I initially reported a larger difference, but after re-running a bunch more times under fair conditions it looks like it's smaller.)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.695% when pulling 4ee46be on asolove:faster-explicit-if into 8567c8b on developit:master.

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Might even be able to kill the vchildren check now that props.children is always an array!

@marvinhagemeister
Copy link
Member

@bmeurer just held a talk about these a few days ago. See the last two slides of his talk where he shares that if-statements without type casting can be effectively 15% faster. How much that relates to real-world performance for a whole library is a different number though.

https://docs.google.com/presentation/d/1_eLlVzcj94_G4r9j9d_Lj5HRKFnq6jgpuPJtnmIBs88/edit?usp=sharing

@@ -187,10 +187,10 @@ function innerDiffNode(dom, vchildren, context, mountAll, absorb) {
min = 0,
len = originalChildren.length,
childrenLen = 0,
vlen = vchildren && vchildren.length,
vlen = vchildren ? vchildren.length : 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the real culprit, as now vlen will always be a Number (and known to the compiler as such). You could probably go one step further and avoid the ToBoolean on vchildren as well by writing something like vlen = (vchildren !== undefined) ? vchildren.length : 0 if that matches the contract.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your insights, @bmeurer!

Would an undefined default (e.g.vlen = vchildren ? vchildren.length : undefined) be less optimal for the compiler than the default of 0? Same question for null. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined or null are less ideal than 0 here, as the compiler needs to choose a tagged representation that can represents both undefined/null as well as numbers. If you use only (small integer) numbers, then the optimizing compilers in VMs can usually pick the ideal unboxed Word/Word32 representation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thank you!

@bmeurer
Copy link
Contributor

bmeurer commented Mar 29, 2017

In general using && or || in a non-boolean context - especially with numbers - is not ideal, because of the semantics of && and || in JavaScript. For example:

let len = o && o.length;

Here len can have either the value of o if ToBoolean(o) yields false, or the value of length property of o. If len is used as a number, then this will prevent the compiler from picking an ideal representation for len, and requires another (expensive) check later to figure out that len is actually a number.

@kentcdodds
Copy link

kentcdodds commented Mar 29, 2017

Interesting! @bmeurer, what if I do this instead:

let len = Boolean(o && o.length);

@bmeurer
Copy link
Contributor

bmeurer commented Mar 29, 2017

Then len will be true or false. :-)

@kentcdodds
Copy link

Doh! Haha, that's embarrassing 😅
Reason I asked was I'm preparing a workshop to teach about ASTs and one of my example babel plugins converts this:

var x = y ? true : z

To this:

var x = Boolean(y) || z

And for some reason I saw this and thought it would be related. 😅

@bmeurer
Copy link
Contributor

bmeurer commented Mar 29, 2017

I have the gut feeling that the first example is better.

@kentcdodds
Copy link

Ha! Maybe I should change the transformation around then!

@bmeurer
Copy link
Contributor

bmeurer commented Mar 29, 2017

Yes, the ?: version is better.

@developit
Copy link
Member

@bmeurer just think of generalizations - using a ternary seems to avoid an unintentionally different type for falsey conditions - seems like that's the kicker here? (the goal being to have the expression produce a uniform type)

@bmeurer
Copy link
Contributor

bmeurer commented Mar 29, 2017

@developit That too!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.695% when pulling 2098502 on asolove:faster-explicit-if into 1444485 on developit:master.

j, c, vchild, child;

if (len) {
if (len!==0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're making things more explicit, don't you really mean len > 0 in both cases?

Copy link
Contributor Author

@asolove asolove Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using > and < does valueOf coercion on non-numeric values, so I have a vague guess that doing an exact equals check may be faster than greater-than. But, I did not actually profile and am not an expert on this, so if someone else knows better please correct me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict equality should be faster than a comparison since it does no casting, but @bmeurer is far more knowledgeable on that than I am.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now. You're focusing on perf over explicit code intent. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the ?: above the optimizing compiler knows that len is a number, so len > 0 is fine. Shouldn't be any speed difference for this. Even the interpreted code is mostly equivalent from perf perspective.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.695% when pulling b8e367f on asolove:faster-explicit-if into 9d9c232 on developit:master.

@robertknight robertknight self-assigned this Apr 2, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.695% when pulling 052a1e0 on asolove:faster-explicit-if into 1efecfb on developit:master.

@robertknight
Copy link
Member

LGTM. Thank-you for the detailed insights @bmeurer .

@robertknight robertknight merged commit 37e5a6f into preactjs:master Apr 2, 2017
@robertknight robertknight removed their assignment Apr 2, 2017
@kurtextrem
Copy link

Wouldn't it make sense to switch len!==0 to len > 0 as per @bmeurer?

@developit
Copy link
Member

@kurtextrem I'd base it on whichever is smaller when gzipped. If I had to guess, the inequality might be smaller because it's used elsewhere in the codebase (but I often guess wrong).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants