Skip to content

Conversation

@mscdex
Copy link
Contributor

@mscdexmscdex commented Dec 25, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • buffer
Description of change

Improve Buffer allocation performance (up to ~11%) by making assertSize() inlineable and improve Buffer.from(arrayLike) performance (~50%) by replacing the usage of the in operator when checking for a length property.

Relevant benchmark results:

Buffer allocation:

 improvement significant p.value buffers/buffer-creation.js n=1024 len=10 type="buffer()" 3.67 % *** 1.907380e-31 buffers/buffer-creation.js n=1024 len=10 type="fast-alloc-fill" 4.34 % *** 3.497544e-11 buffers/buffer-creation.js n=1024 len=10 type="fast-alloc" 9.94 % *** 7.596522e-44 buffers/buffer-creation.js n=1024 len=10 type="fast-allocUnsafe" 5.05 % *** 2.696345e-51 buffers/buffer-creation.js n=1024 len=10 type="slow-allocUnsafe" 3.49 % *** 1.076653e-07 buffers/buffer-creation.js n=1024 len=10 type="slow" 11.22 % *** 5.188153e-61 buffers/buffer-creation.js n=1024 len=1024 type="buffer()" 1.42 % ** 1.348395e-03 buffers/buffer-creation.js n=1024 len=1024 type="fast-alloc-fill" 2.52 % *** 1.446582e-06 buffers/buffer-creation.js n=1024 len=1024 type="fast-alloc" 5.38 % *** 1.283277e-27 buffers/buffer-creation.js n=1024 len=1024 type="fast-allocUnsafe" 2.18 % *** 2.896226e-06 buffers/buffer-creation.js n=1024 len=1024 type="slow-allocUnsafe" 3.41 % *** 6.929603e-10 buffers/buffer-creation.js n=1024 len=1024 type="slow" 9.38 % *** 1.607505e-56 buffers/buffer-creation.js n=1024 len=2048 type="buffer()" 2.13 % *** 2.849077e-05 buffers/buffer-creation.js n=1024 len=2048 type="fast-alloc-fill" 2.39 % *** 1.291052e-07 buffers/buffer-creation.js n=1024 len=2048 type="fast-alloc" 3.96 % *** 2.701098e-22 buffers/buffer-creation.js n=1024 len=2048 type="fast-allocUnsafe" 1.57 % *** 5.975363e-04 buffers/buffer-creation.js n=1024 len=2048 type="slow-allocUnsafe" 4.54 % *** 3.838296e-15 buffers/buffer-creation.js n=1024 len=2048 type="slow" 8.52 % *** 6.733714e-46 buffers/buffer-creation.js n=1024 len=4096 type="buffer()" 3.62 % *** 1.399411e-10 buffers/buffer-creation.js n=1024 len=4096 type="fast-alloc-fill" 1.85 % *** 2.706257e-05 buffers/buffer-creation.js n=1024 len=4096 type="fast-alloc" 1.30 % *** 1.625928e-04 buffers/buffer-creation.js n=1024 len=4096 type="fast-allocUnsafe" 4.14 % *** 4.122709e-15 buffers/buffer-creation.js n=1024 len=4096 type="slow-allocUnsafe" 3.78 % *** 5.601502e-13 buffers/buffer-creation.js n=1024 len=4096 type="slow" 8.09 % *** 6.466906e-46 buffers/buffer-creation.js n=1024 len=8192 type="buffer()" 2.54 % *** 1.426874e-05 buffers/buffer-creation.js n=1024 len=8192 type="fast-alloc-fill" 1.16 % ** 1.392665e-03 buffers/buffer-creation.js n=1024 len=8192 type="fast-alloc" 0.83 % ** 2.391328e-03 buffers/buffer-creation.js n=1024 len=8192 type="fast-allocUnsafe" 5.33 % *** 1.811794e-19 buffers/buffer-creation.js n=1024 len=8192 type="slow-allocUnsafe" 4.85 % *** 1.764478e-16 buffers/buffer-creation.js n=1024 len=8192 type="slow" 8.18 % *** 1.006915e-42 

Buffer.from(arrayLike):

 improvement significant p.value buffers/buffer-from.js n=10024 len=10 source="object" 48.85 % *** 4.055606e-41 buffers/buffer-from.js n=10024 len=2048 source="object" 49.44 % *** 2.809991e-51 

/cc @nodejs/buffer
CI: https://ci.nodejs.org/job/node-test-pull-request/5580/

@mscdexmscdex added the buffer Issues and PRs related to the buffer subsystem. label Dec 25, 2016
@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. dont-land-on-v7.x labels Dec 25, 2016
@mscdexmscdex added the performance Issues and PRs related to the performance of Node.js. label Dec 25, 2016
@mscdexmscdex changed the title Buffer allocate perfbuffer: improve allocation and from(arrayLike) performanceDec 25, 2016
lib/buffer.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Nope. Removed.

Copy link
Contributor

@cjihrigcjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a question.

assertSize() is adjusted to be inlineable according to V8's default function size limits when determining inlineability. This results in up to 11% performance gains when allocating any kind of Buffer.
This change results in ~50% improvement when creating a Buffer from an array-like object.
@Trott
Copy link
Member

Trott commented Dec 25, 2016

Probably too tangentially related to this PR to be worth doing, but just in case: There's a != on line 172 of lib/buffer.js that seems to be calling out to be replaced with !==.

Copy link
Member

@ChALkeRChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM after CI.

@jasnell
Copy link
Member

jasnell pushed a commit that referenced this pull request Dec 27, 2016
assertSize() is adjusted to be inlineable according to V8's default function size limits when determining inlineability. This results in up to 11% performance gains when allocating any kind of Buffer. Avoid avoids use of in, resulting in ~50% improvement when creating a Buffer from an array-like object. PR-URL: #10443 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member

Landed in 13a4887

@jasnelljasnell closed this Dec 27, 2016
@mscdexmscdex deleted the buffer-allocate-perf branch December 29, 2016 02:20
@mscdexmscdex mentioned this pull request Dec 31, 2016
3 tasks
@evanlucasevanlucas mentioned this pull request Jan 3, 2017
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
assertSize() is adjusted to be inlineable according to V8's default function size limits when determining inlineability. This results in up to 11% performance gains when allocating any kind of Buffer. Avoid avoids use of in, resulting in ~50% improvement when creating a Buffer from an array-like object. PR-URL: #10443 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
evanlucas added a commit that referenced this pull request Jan 3, 2017
Notable changes: * buffer: - Improve performance of Buffer allocation by ~11% (Brian White) #10443 - Improve performance of Buffer.from() by ~50% (Brian White) #10443 * events: Improve performance of EventEmitter.once() by ~27% (Brian White) #10445 * http: Improve performance of http server by ~7% (Brian White) #6533
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
assertSize() is adjusted to be inlineable according to V8's default function size limits when determining inlineability. This results in up to 11% performance gains when allocating any kind of Buffer. Avoid avoids use of in, resulting in ~50% improvement when creating a Buffer from an array-like object. PR-URL: #10443 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
evanlucas added a commit that referenced this pull request Jan 4, 2017
Notable changes: * buffer: - Improve performance of Buffer allocation by ~11% (Brian White) #10443 - Improve performance of Buffer.from() by ~50% (Brian White) #10443 * events: Improve performance of EventEmitter.once() by ~27% (Brian White) #10445 * fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) #10382 * http: Improve performance of http server by ~7% (Brian White) #6533 * npm: Upgrade to v4.0.5 (Kat Marchán) #10330 PR-URL: #10589
evanlucas added a commit that referenced this pull request Jan 4, 2017
Notable changes: * buffer: - Improve performance of Buffer allocation by ~11% (Brian White) #10443 - Improve performance of Buffer.from() by ~50% (Brian White) #10443 * events: Improve performance of EventEmitter.once() by ~27% (Brian White) #10445 * fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) #10382 * http: Improve performance of http server by ~7% (Brian White) #6533 * npm: Upgrade to v4.0.5 (Kat Marchán) #10330 PR-URL: #10589
@MylesBorins
Copy link
Contributor

Adding LTS watch, likely should bake a bit longer before landing.

imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
 Notable changes: * buffer: - Improve performance of Buffer allocation by ~11% (Brian White) nodejs/node#10443 - Improve performance of Buffer.from() by ~50% (Brian White) nodejs/node#10443 * events: Improve performance of EventEmitter.once() by ~27% (Brian White) nodejs/node#10445 * fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) nodejs/node#10382 * http: Improve performance of http server by ~7% (Brian White) nodejs/node#6533 * npm: Upgrade to v4.0.5 (Kat Marchán) nodejs/node#10330 PR-URL: nodejs/node#10589 Signed-off-by: Ilkka Myller <[email protected]>
@MylesBorinsMylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. and removed lts-watch-v4.x labels May 8, 2017
@MylesBorinsMylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bufferIssues and PRs related to the buffer subsystem.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@mscdex@Trott@jasnell@MylesBorins@ChALkeR@addaleax@lpinca@cjihrig@nodejs-github-bot