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

buffer: improve Buffer.from performance #15178

Closed

Conversation

apapirovski
Copy link
Member

Using == null in code paths that are expected to mostly receive objects, arrays or other more complex data types is not great because typecasting these types is very slow. Change to instead check === null || === undefined.

Here's the benchmark:

buffers/buffer-from.js n=2048 len=10 source="array"                     7.26 %        *** 4.653123e-04
buffers/buffer-from.js n=2048 len=10 source="arraybuffer-middle"        2.50 %            1.937961e-01
buffers/buffer-from.js n=2048 len=10 source="arraybuffer"               0.57 %            7.604131e-01
buffers/buffer-from.js n=2048 len=10 source="buffer"                    4.23 %            5.768482e-02
buffers/buffer-from.js n=2048 len=10 source="object"                   16.31 %        *** 1.128762e-09
buffers/buffer-from.js n=2048 len=10 source="string-base64"             0.08 %            9.655769e-01
buffers/buffer-from.js n=2048 len=10 source="string-utf8"              -1.90 %            2.904045e-01
buffers/buffer-from.js n=2048 len=10 source="string"                   -0.80 %            6.723524e-01
buffers/buffer-from.js n=2048 len=10 source="uint8array"                3.63 %            5.416784e-02
buffers/buffer-from.js n=2048 len=2048 source="array"                   1.42 %            4.821207e-01
buffers/buffer-from.js n=2048 len=2048 source="arraybuffer-middle"     -1.04 %            7.025216e-01
buffers/buffer-from.js n=2048 len=2048 source="arraybuffer"             0.36 %            8.475033e-01
buffers/buffer-from.js n=2048 len=2048 source="buffer"                  0.99 %            7.272932e-01
buffers/buffer-from.js n=2048 len=2048 source="object"                 18.59 %        *** 1.131309e-11
buffers/buffer-from.js n=2048 len=2048 source="string-base64"          -0.17 %            9.388870e-01
buffers/buffer-from.js n=2048 len=2048 source="string-utf8"            -0.35 %            8.910105e-01
buffers/buffer-from.js n=2048 len=2048 source="string"                  0.30 %            9.041955e-01
buffers/buffer-from.js n=2048 len=2048 source="uint8array"              0.41 %            8.854981e-01

and here's the old vs old, just to show that the string slowdown is probably just within margin of error:

buffers/buffer-from.js n=2048 len=10 source="array"                    -0.27 %            0.9122062
buffers/buffer-from.js n=2048 len=10 source="arraybuffer-middle"       -0.50 %            0.8118019
buffers/buffer-from.js n=2048 len=10 source="arraybuffer"               1.77 %            0.5688189
buffers/buffer-from.js n=2048 len=10 source="buffer"                    0.98 %            0.7024176
buffers/buffer-from.js n=2048 len=10 source="object"                    0.92 %            0.7081286
buffers/buffer-from.js n=2048 len=10 source="string-base64"            -0.92 %            0.7706629
buffers/buffer-from.js n=2048 len=10 source="string-utf8"              -3.24 %            0.1525157
buffers/buffer-from.js n=2048 len=10 source="string"                    1.52 %            0.4168377
buffers/buffer-from.js n=2048 len=10 source="uint8array"               -0.42 %            0.8494492
buffers/buffer-from.js n=2048 len=2048 source="array"                   0.71 %            0.7683624
buffers/buffer-from.js n=2048 len=2048 source="arraybuffer-middle"      1.05 %            0.5941559
buffers/buffer-from.js n=2048 len=2048 source="arraybuffer"             1.96 %            0.4494948
buffers/buffer-from.js n=2048 len=2048 source="buffer"                 -0.57 %            0.7376221
buffers/buffer-from.js n=2048 len=2048 source="object"                 -1.35 %            0.5914752
buffers/buffer-from.js n=2048 len=2048 source="string-base64"           0.93 %            0.7160677
buffers/buffer-from.js n=2048 len=2048 source="string-utf8"             0.32 %            0.8848240
buffers/buffer-from.js n=2048 len=2048 source="string"                  0.77 %            0.7390453
buffers/buffer-from.js n=2048 len=2048 source="uint8array"             -0.40 %            0.8145751

For reference, == vs === is about 10x slower: https://jsperf.com/triple-equals-vs-double-equals/3

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Using == null in code paths that are expected to mostly receive
objects, arrays or other more complex data types is not
ideal because typecasting these types is very slow. Change
to instead check === null || === undefined. Also move one
variable assignment in fromString after an if condition
that doesn't need it (and returns if truthy).

Refs: https://jsperf.com/triple-equals-vs-double-equals/3
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Sep 4, 2017
@apapirovski
Copy link
Member Author

There are other parts of code outside of Buffer that extensively use == null or != null, some of that isn't code that runs often but then there's stuff like encoding.js and url.js which seems performance sensitive and should probably not be using it. Would people prefer a separate PR for each module or just one? Or none at all?

@targos
Copy link
Member

targos commented Sep 4, 2017

/cc @bmeurer what do you think? I'd expect this to be optimizable by the engine but maybe I'm missing something about the spec?

@bmeurer
Copy link
Member

bmeurer commented Sep 4, 2017

Node doesn't have document.all, so ==null could probably be made as fast as the explicit ==null||==undefined. But the performance behavior will be different from that in Chrome then. Is that something we'd like to have?

cc @psmarshall

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Sep 4, 2017
@jasnell jasnell requested a review from mscdex September 5, 2017 17:14
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

The changes LGTM but would like @mscdex to sign off also

@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

@Jessidhia
Copy link

(Probably off-topic but) what about document.all necessitates being pessimistic about == null checks in the optimizer?

@apapirovski
Copy link
Member Author

@Kovensky see https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all for more info

@Jessidhia
Copy link

I see (a quick search just said that was non-standard and “do not use”). Thanks for the reference.

Maybe one day it could be optimized in browsers too by only being pessimistic if document.all is ever accessed, but that is definitely an off-topic discussion.

@TimothyGu
Copy link
Member

@Kovensky, FWIW also see tc39/ecma262#673, which brings the odd behaviors of document.all into Annex B, so technically "spec-compliant". See latest spec. Since that PR was merged pretty recently, the integration of that ECMAScript to the HTML Standard is pending (whatwg/html#3015).

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

@jasnell as this is a very simple patch and it is clear what is going on I hope it is fine to land this even without the LG of @mscdex? (I do not want to keep anyone from looking at this though 😆)

@apapirovski
Copy link
Member Author

I think it might be good to resolve what @targos & @bmeurer brought up above. If this can be optimized away in V8 then that might be preferable... ?

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

It would be great if v8 handles this better but it will take a while until this would get into Node.js. This is a trivial fix that improves the current situation and therefore I would definitely like to land this.

@mscdex
Copy link
Contributor

mscdex commented Sep 9, 2017

It would be nice to have V8 optimize the single equals case for null/undefined, unless they can implement something that sees the double check pattern and optimizes it as a special case.

@BridgeAR
Copy link
Member

@mscdex but you do not mind landing this for the time being, do you? Because if you are good with this, it could land.

@mscdex
Copy link
Contributor

mscdex commented Sep 11, 2017

@BridgeAR It's fine for now I suppose.

@BridgeAR
Copy link
Member

Landed in fc1fa4e

@BridgeAR BridgeAR closed this Sep 11, 2017
BridgeAR pushed a commit that referenced this pull request Sep 11, 2017
Using == null in code paths that are expected to mostly receive
objects, arrays or other more complex data types is not
ideal because typecasting these types is very slow. Change
to instead check === null || === undefined. Also move one
variable assignment in fromString after an if condition
that doesn't need it (and returns if truthy).

PR-URL: #15178
Refs: https://jsperf.com/triple-equals-vs-double-equals/3
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Using == null in code paths that are expected to mostly receive
objects, arrays or other more complex data types is not
ideal because typecasting these types is very slow. Change
to instead check === null || === undefined. Also move one
variable assignment in fromString after an if condition
that doesn't need it (and returns if truthy).

PR-URL: nodejs#15178
Refs: https://jsperf.com/triple-equals-vs-double-equals/3
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
Using == null in code paths that are expected to mostly receive
objects, arrays or other more complex data types is not
ideal because typecasting these types is very slow. Change
to instead check === null || === undefined. Also move one
variable assignment in fromString after an if condition
that doesn't need it (and returns if truthy).

PR-URL: #15178
Refs: https://jsperf.com/triple-equals-vs-double-equals/3
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Oct 17, 2017
@MylesBorins
Copy link
Contributor

should this land in LTS? If so it will need to bake a bit longer.

Please change labels as appropriate

@apapirovski
Copy link
Member Author

apapirovski commented Oct 17, 2017

Not certain. I'll need to run benchmarks to see if this change makes a difference on v6.x given the different V8. That said, if it does make a difference this might be possible to backport almost immediately since it doesn't functionally change anything just makes the check more V8 friendly.

@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants