buffer: improve Buffer.from performance by apapirovski · Pull Request #15178 · nodejs/node · GitHub

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

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?

/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?

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

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

CI: https://ci.nodejs.org/job/node-test-pull-request/9971/

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

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

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.

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

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

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

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.

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.

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

@BridgeAR It's fine for now I suppose.

Landed in fc1fa4e

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>

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>

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>

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

Please change labels as appropriate

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.

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Sep 4, 2017

apapirovski force-pushed the patch-buffer-from-perf branch from 99f480d to f9fbc3a Compare September 4, 2017 12:54

BridgeAR approved these changes Sep 4, 2017

View reviewed changes

mscdex added the performance Issues and PRs related to the performance of Node.js. label Sep 4, 2017

jasnell requested a review from mscdex September 5, 2017 17:14

jasnell approved these changes Sep 7, 2017

View reviewed changes

TimothyGu approved these changes Sep 11, 2017

View reviewed changes

BridgeAR closed this Sep 11, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

BridgeAR mentioned this pull request Sep 21, 2017

fs: initial experimental promisified API #15485

Closed

4 tasks

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Oct 17, 2017

MylesBorins added the lts-watch-v6.x label Oct 17, 2017

Jessidhia mentioned this pull request Oct 18, 2017

Implement transform for nullish-coalescing operator babel/babel#6483

Merged

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

buffer: improve Buffer.from performance by apapirovski · Pull Request #15178 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: improve Buffer.from performance - #15178

buffer: improve Buffer.from performance#15178apapirovski wants to merge 1 commit intonodejs:masternodejs/node:masterfrom apapirovski:patch-buffer-from-perfapapirovski/node:patch-buffer-from-perfCopy head branch name to clipboard

Conversation

apapirovski commented Sep 4, 2017

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski commented Sep 4, 2017

Uh oh!

targos commented Sep 4, 2017

Uh oh!

bmeurer commented Sep 4, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 7, 2017

Uh oh!

Jessidhia commented Sep 8, 2017

Uh oh!

apapirovski commented Sep 8, 2017

Uh oh!

Jessidhia commented Sep 8, 2017

Uh oh!

TimothyGu commented Sep 8, 2017

Uh oh!

BridgeAR commented Sep 9, 2017

Uh oh!

apapirovski commented Sep 9, 2017

Uh oh!

BridgeAR commented Sep 9, 2017

Uh oh!

mscdex commented Sep 9, 2017

Uh oh!

BridgeAR commented Sep 11, 2017

Uh oh!

mscdex commented Sep 11, 2017

Uh oh!

BridgeAR commented Sep 11, 2017

Uh oh!

MylesBorins commented Oct 17, 2017

Uh oh!

apapirovski commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

buffer: improve Buffer.from performance#15178
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-buffer-from-perf

apapirovski commented Oct 17, 2017 •
edited
Loading