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.

Sign up for GitHub

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

Jump to bottom

buffer: use slightly faster NaN check #12286

Merged
merged 1 commit into from
Apr 14, 2017
Merged

buffer: use slightly faster NaN check #12286

merged 1 commit into from
Apr 14, 2017

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
'use strict';
var common = require('../common.js');
var fs = require('fs');
const path = require('path');
Copy link
Contributor

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

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

@aqrln aqrln aqrln left review comments

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

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