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

querystring: improve performance #29306

Merged
merged 0 commits into from
Aug 28, 2019
Merged

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Aug 25, 2019

Results:

                                                                   confidence improvement accuracy (*)     (**)    (***)
 querystring/querystring-parse.js n=1000000 type='altspaces'             ***    985.03 %      ±27.88%  ±40.04%  ±58.85%
 querystring/querystring-parse.js n=1000000 type='encodefake'            ***   1253.69 %      ±26.56%  ±38.14%  ±56.07%
 querystring/querystring-parse.js n=1000000 type='encodelast'            ***   1012.30 %      ±26.81%  ±38.49%  ±56.56%
 querystring/querystring-parse.js n=1000000 type='encodemany'            ***    712.02 %      ±34.90%  ±50.11%  ±73.65%
 querystring/querystring-parse.js n=1000000 type='manypairs'             ***      4.42 %       ±1.35%   ±1.85%   ±2.52%
 querystring/querystring-parse.js n=1000000 type='multicharsep'          ***   1053.69 %      ±21.01%  ±30.15%  ±44.28%
 querystring/querystring-parse.js n=1000000 type='multivalue'            ***   1161.95 %      ±41.03%  ±58.93%  ±86.66%
 querystring/querystring-parse.js n=1000000 type='noencode'              ***   1094.45 %      ±96.37% ±138.44% ±203.66%

 querystring/querystring-stringify.js n=1000000 type='array'             ***      8.28 %       ±2.20%   ±2.94%   ±3.84%
 querystring/querystring-stringify.js n=1000000 type='noencode'           **      3.38 %       ±2.43%   ±3.24%   ±4.21%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mscdex mscdex added querystring Issues and PRs related to the built-in querystring module. performance Issues and PRs related to the performance of Node.js. labels Aug 25, 2019
@nodejs-github-bot
Copy link
Collaborator

@mscdex
Copy link
Contributor Author

mscdex commented Aug 25, 2019

I believe this may be applicable to v10.x as well (in addition to v12.x of course), but I have not yet tested.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Do you know why the perf difference is so large? It doesn’t look like there are any substantial changes to the code itself, which makes me curious…

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I'm also curious. I would expect preallocating the charCodes() array to be 2-3x faster but that only explains the multicharsep benchmark and that still doesn't explain the 10x speedup. :-)

Nice work at any rate!

@mscdex
Copy link
Contributor Author

mscdex commented Aug 25, 2019

I didn't dig that deep to find out why but I think V8 is not optimizing the parse() function? I also noticed that node v8.x is ~5x faster or so than node v10.x+ when it comes to parse() (without these changes).

@ZYSzys ZYSzys added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 26, 2019
lib/querystring.js Outdated Show resolved Hide resolved
@mscdex mscdex closed this Aug 28, 2019
@mscdex mscdex merged commit 248e9ec into nodejs:master Aug 28, 2019
@mscdex mscdex deleted the querystring-perf branch August 28, 2019 07:14
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
PR-URL: #29306
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
PR-URL: #29306
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. performance Issues and PRs related to the performance of Node.js. querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants