Skip to content

Conversation

@Trott
Copy link
Member

@TrottTrott commented May 16, 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: #12153
Reviewed-By: Joyee Cheung
Reviewed-By: Michaël Zasso
Reviewed-By: James M Snell

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@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. v6.x labels May 16, 2017
@TrottTrott mentioned this pull request May 16, 2017
2 tasks
@MylesBorins
Copy link
Contributor

are there benchmarks to show this improves perf in v6.x?

@sam-github
Copy link
Contributor

"definied" <--- typo in commit message

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]>
@TrottTrottforce-pushed the backport-12153-again branch from 636e55d to c2abbd1CompareMay 19, 2017 05:13
@Trott
Copy link
MemberAuthor

are there benchmarks to show this improves perf in v6.x?

@MylesBorins Much to my surprise, this change that had measurable positive performance impact on master appears to have the opposite impact here:

 improvement confidence p.value buffers/buffer-from.js n=2048 len=10 source="arraybuffer-middle" -6.15 % *** 4.991252e-14 buffers/buffer-from.js n=2048 len=10 source="arraybuffer" -7.13 % *** 5.563436e-17 buffers/buffer-from.js n=2048 len=2048 source="arraybuffer-middle" -5.51 % *** 5.312334e-15 buffers/buffer-from.js n=2048 len=2048 source="arraybuffer" -6.82 % *** 4.554484e-14

I haven't looked into why, but for now, I'm going to close this and add a do-not-land-on-v6.x label to the original PR.

@TrottTrott closed this May 19, 2017
@TrottTrott deleted the backport-12153-again 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.

5 participants

@Trott@MylesBorins@sam-github@thefourtheye@nodejs-github-bot