Skip to content

buffer: use slightly faster NaN check#12286

Merged
mscdex merged 1 commit intonodejs:masterfrom
mscdex:buffer-faster-isnan
Apr 14, 2017
Merged

buffer: use slightly faster NaN check#12286
mscdex merged 1 commit intonodejs:masterfrom
mscdex:buffer-faster-isnan

Conversation

@mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 8, 2017 •
edited
Loading

Performance results:

                                                       improvement confidence      p.value
 buffers/buffer-indexof-number.js n=10000000 value=64      3.59 %        *** 5.794893e-21
 buffers/buffer-slice.js n=8192 type="fast"                6.66 %        *** 1.325662e-23
 buffers/buffer-slice.js n=8192 type="slow"                7.45 %        *** 5.384460e-23

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

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

@mscdex mscdex added buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js. labels Apr 8, 2017
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Apr 8, 2017
aqrln
aqrln reviewed Apr 8, 2017
View reviewed changes
benchmark/buffers/buffer-indexof-number.js Outdated
Copy link
Contributor

@aqrln aqrln Apr 8, 2017

Choose a reason for hiding this comment

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

Nit: inconsistent var and const usage. I suppose it was copied from benchmark/buffers/buffer-indexof.js.

Copy link
Contributor Author

@mscdex mscdex Apr 14, 2017

Choose a reason for hiding this comment

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

Fixed.

mhdawson
mhdawson approved these changes Apr 13, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex mscdex force-pushed the buffer-faster-isnan branch from 8d08d6e to ab5b9ee Compare April 14, 2017 03:12
@mscdex mscdex force-pushed the buffer-faster-isnan branch from ab5b9ee to 667ea63 Compare April 14, 2017 21:20
@mscdex
Copy link
Contributor Author

mscdex commented Apr 14, 2017

One last CI before landing: https://ci.nodejs.org/job/node-test-pull-request/7411/

@mscdex
buffer: use slightly faster NaN check
e0f0f26 
PR-URL: nodejs#12286
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott Trott mentioned this pull request Apr 14, 2017
Closed
4 tasks
@mscdex mscdex force-pushed the buffer-faster-isnan branch from 667ea63 to e0f0f26 Compare April 14, 2017 22:05
@mscdex mscdex merged commit e0f0f26 into nodejs:master Apr 14, 2017
@mscdex mscdex deleted the buffer-faster-isnan branch April 14, 2017 22:07
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@mscdex @evanlucas
buffer: use slightly faster NaN check
ab963c1 
PR-URL: #12286
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@mscdex @evanlucas
buffer: use slightly faster NaN check
30541bf 
PR-URL: #12286
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@mscdex @evanlucas
buffer: use slightly faster NaN check
e4b2f61 
PR-URL: #12286
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gibfahn gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels May 15, 2017
@gibfahn
Copy link
Member

gibfahn commented May 15, 2017

@mscdex should this be backported to v6.x?

@mscdex
Copy link
Contributor Author

mscdex commented May 15, 2017

@gibfahn I'm pretty sure it's applicable performance-wise (even back to the v0.10 days) from what I remember.

@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

+2 more reviewers

@aqrln aqrln aqrln left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@mscdex @gibfahn @jasnell @lpinca @TimothyGu @cjihrig @aqrln @mhdawson @MylesBorins @nodejs-github-bot