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

(v6.x backport) buffer,util: refactor for performance #13046

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott 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-bot nodejs-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
@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 <joyeec9h3@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott force-pushed the backport-12153-again branch from 636e55d to c2abbd1 Compare May 19, 2017 05:13
@Trott
Copy link
Member Author

Trott commented May 19, 2017

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.

@Trott Trott closed this May 19, 2017
@Trott Trott deleted the backport-12153-again branch January 13, 2022 22:45
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. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants