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

Projects
None yet
10 participants
@asolove
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 26, 2017

Coverage Status

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

coveralls commented Mar 26, 2017

Coverage Status

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

@developit

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

@marvinhagemeister

This comment has been minimized.

Show comment
Hide comment
@marvinhagemeister

marvinhagemeister Mar 28, 2017

Collaborator

@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

Collaborator

marvinhagemeister commented Mar 28, 2017

@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,

This comment has been minimized.

@bmeurer

bmeurer Mar 29, 2017

Contributor

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.

@bmeurer

bmeurer Mar 29, 2017

Contributor

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.

This comment has been minimized.

@kylpo

kylpo Mar 29, 2017

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!

@kylpo

kylpo Mar 29, 2017

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!

This comment has been minimized.

@bmeurer

bmeurer Mar 29, 2017

Contributor

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.

@bmeurer

bmeurer Mar 29, 2017

Contributor

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.

This comment has been minimized.

@kylpo

kylpo Mar 29, 2017

Got it. Thank you!

@kylpo

kylpo Mar 29, 2017

Got it. Thank you!

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Mar 29, 2017

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@kentcdodds

kentcdodds Mar 29, 2017

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

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

kentcdodds commented Mar 29, 2017

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

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

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Mar 29, 2017

Contributor

Then len will be true or false. :-)

Contributor

bmeurer commented Mar 29, 2017

Then len will be true or false. :-)

@kentcdodds

This comment has been minimized.

Show comment
Hide comment
@kentcdodds

kentcdodds Mar 29, 2017

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. 😅

kentcdodds commented Mar 29, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Mar 29, 2017

Contributor

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

Contributor

bmeurer commented Mar 29, 2017

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

@kentcdodds

This comment has been minimized.

Show comment
Hide comment
@kentcdodds

kentcdodds Mar 29, 2017

Ha! Maybe I should change the transformation around then!

kentcdodds commented Mar 29, 2017

Ha! Maybe I should change the transformation around then!

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Mar 29, 2017

Contributor

Yes, the ?: version is better.

Contributor

bmeurer commented Mar 29, 2017

Yes, the ?: version is better.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Mar 29, 2017

Owner

@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)

Owner

developit commented Mar 29, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Mar 29, 2017

Contributor

@developit That too!

Contributor

bmeurer commented Mar 29, 2017

@developit That too!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 29, 2017

Coverage Status

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

coveralls commented Mar 29, 2017

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) {

This comment has been minimized.

@fearphage

fearphage Mar 29, 2017

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

@fearphage

fearphage Mar 29, 2017

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

This comment has been minimized.

@asolove

asolove Mar 29, 2017

Contributor

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.

@asolove

asolove Mar 29, 2017

Contributor

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.

This comment has been minimized.

@developit

developit Mar 30, 2017

Owner

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

@developit

developit Mar 30, 2017

Owner

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

This comment has been minimized.

@fearphage

fearphage Mar 30, 2017

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

@fearphage

fearphage Mar 30, 2017

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

This comment has been minimized.

@bmeurer

bmeurer Mar 30, 2017

Contributor

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.

@bmeurer

bmeurer Mar 30, 2017

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

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

coveralls commented Mar 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 2, 2017

Coverage Status

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

coveralls commented Apr 2, 2017

Coverage Status

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

@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Apr 2, 2017

Collaborator

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

Collaborator

robertknight commented Apr 2, 2017

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

@robertknight robertknight merged commit 37e5a6f into developit:master Apr 2, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 97.695%
Details
security/snyk No new vulnerabilities
Details

@robertknight robertknight removed their assignment Apr 2, 2017

@kurtextrem

This comment has been minimized.

Show comment
Hide comment
@kurtextrem

kurtextrem Apr 3, 2017

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

kurtextrem commented Apr 3, 2017

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

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Apr 3, 2017

Owner

@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).

Owner

developit commented Apr 3, 2017

@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).

@tomdale tomdale referenced this pull request Apr 3, 2017

Merged

Test and refactor rendering #40

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