Skip to content

Conversation

@Trott
Copy link
Member

@TrottTrott commented Apr 1, 2017

internal/util.js definied toInteger() and toLength() but they were only
used by lib/buffer.js. Inlining these small functions results in a small but
statistically-significant performance gain.

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

buffer util

@TrottTrott added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. labels Apr 1, 2017
@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. labels Apr 1, 2017
internal/util.js definied toInteger() and toLength() but they were only used by buffer.js. Inlining these small functions results in a small but statistically-significant performance gain.
@Trott
Copy link
MemberAuthor

Trott commented Apr 1, 2017

$ node benchmark/compare.js --old /var/tmp/node-master --new /var/tmp/node-new --filter buffer-from --set 'source=arraybuffer' --set 'source=arraybuffer-middle' buffers > /var/tmp/compare.csv[00:01:09|% 100| 1/1 files | 60/60 runs | 4/4 configs]: Done $ cat /var/tmp/compare.csv | Rscript benchmark/compare.R  improvement confidence p.value buffers/buffer-from.js n=1024 len=10 source="arraybuffer-middle" 7.12 % *** 1.415439e-18 buffers/buffer-from.js n=1024 len=10 source="arraybuffer" 0.05 % 9.482883e-01 buffers/buffer-from.js n=1024 len=2048 source="arraybuffer-middle" 4.91 % ** 2.134363e-03 buffers/buffer-from.js n=1024 len=2048 source="arraybuffer" 0.94 % 2.267840e-01 $

@Trott
Copy link
MemberAuthor

Trott commented Apr 1, 2017

@cjihrig
Copy link
Contributor

I thought the idea was to use these in other places @thefourtheye ?

@jasnell
Copy link
Member

@cjihrig ... I'm sure it could be, but until there's a need for that, this should be fine. We can revert this if it proves necessary.

jasnell pushed a commit that referenced this pull request Apr 4, 2017
internal/util.js definied toInteger() and toLength() but they were only used by buffer.js. Inlining these small functions results in a small but statistically-significant performance gain. PR-URL: #12153 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

Landed in 1e6186e

@italoacasas
Copy link

cc @Trott

Trott added a commit to Trott/io.js that referenced this pull request Apr 11, 2017
internal/util.js definied toInteger() and toLength() but they were only used by buffer.js. Inlining these small functions results in a small but statistically-significant performance gain. PR-URL: nodejs#12153 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
MemberAuthor

@italoacasas#12328

Trott added a commit to Trott/io.js that referenced this pull request Apr 21, 2017
internal/util.js definied toInteger() and toLength() but they were only used by buffer.js. Inlining these small functions results in a small but statistically-significant performance gain. PR-URL: nodejs#12153 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
@evanlucasevanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
internal/util.js definied toInteger() and toLength() but they were only used by buffer.js. Inlining these small functions results in a small but statistically-significant performance gain. PR-URL: #12153 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 15, 2017
@MylesBorins
Copy link
Contributor

should we backport to v6.x? I've added the baking label as we should wait a bit if it is going to land

@thefourtheye
Copy link
Contributor

I am going to take a wild guess here. The performance improvement seen here could be because of the hidden isNaN check. The idea was to standardise buffer checks and toInteger could be used everywhere else.

@Trott
Copy link
MemberAuthor

@MylesBorins Backport to v6.x PR: #13046

Trott added a commit to Trott/io.js that referenced this pull request May 19, 2017
internal/util.js defined toInteger() and toLength() but they were only used by buffer.js. Inlining these small functions results in a small but statistically-significant performance gain. PR-URL: nodejs#12153 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
@TrottTrott added dont-land-on-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels May 19, 2017
@TrottTrott deleted the buffer-refactor branch January 13, 2022 22:45
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.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@Trott@cjihrig@jasnell@italoacasas@MylesBorins@thefourtheye@targos@joyeecheung@addaleax@nodejs-github-bot