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

querystring: improve unescapeBuffer performance #10837

Merged
merged 1 commit into from
Jan 25, 2017
Merged

querystring: improve unescapeBuffer performance #10837

merged 1 commit into from
Jan 25, 2017

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jan 16, 2017 •
edited
Loading

Results with included benchmark:

                                                                                                                                       improvement confidence      p.value
 querystring/querystring-unescapebuffer.js n=10000000 input="%20%21%22%23%24%25%26%27%28%29%2A%2B%2C%2D%2E%2F%30%31%32%33%34%35%36%37"     12.86 %        *** 5.432023e-29
 querystring/querystring-unescapebuffer.js n=10000000 input="there is nothing to unescape here"                                            18.04 %        *** 6.686024e-40
 querystring/querystring-unescapebuffer.js n=10000000 input="there%20are%20several%20spaces%20that%20need%20to%20be%20unescaped"           21.62 %        *** 1.254710e-42
 querystring/querystring-unescapebuffer.js n=10000000 input="there%2Qare%0-fake%escaped values in%%%%this%9Hstring"                         9.11 %        *** 1.738303e-14

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

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)
  • querystring

@mscdex mscdex added performance Issues and PRs related to the performance of Node.js. querystring Issues and PRs related to the built-in querystring module. labels Jan 16, 2017
@Fishrock123
Copy link
Contributor

Didn't this like, used to use a mapping table like a year ago? haha

@mscdex
Copy link
Contributor Author

mscdex commented Jan 16, 2017

@Fishrock123 Not that I'm aware of. stringify() still uses a (different) table though.

mscdex
mscdex commented Jan 16, 2017
View reviewed changes
lib/querystring.js
for (var inIndex = 0, outIndex = 0; inIndex <= s.length; inIndex++) {
var c = inIndex < s.length ? s.charCodeAt(inIndex) : NaN;
for (var inIndex = 0, outIndex = 0; ; inIndex++) {
if (inIndex < s.length) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, to anyone who might ask why I didn't put this conditional in the for conditional and the else block immediately after the end of the for loop, it's because there is actually a 2% reduction in the performance increase for the 'fake escaped values' input. The results for the other inputs seem to be largely unaffected though.

@mscdex mscdex force-pushed the querystring-unescapebuffer-perf branch from b854ebc to 0cf1054 Compare January 16, 2017 16:26
@mscdex mscdex force-pushed the querystring-unescapebuffer-perf branch from 0cf1054 to 568b353 Compare January 16, 2017 17:05
@mscdex mscdex force-pushed the querystring-unescapebuffer-perf branch from 568b353 to c3c619a Compare January 16, 2017 17:11
@mscdex mscdex force-pushed the querystring-unescapebuffer-perf branch from c3c619a to 3bdcbdb Compare January 25, 2017 07:23
@mscdex mscdex merged commit 3bdcbdb into nodejs:master Jan 25, 2017
@mscdex mscdex deleted the querystring-unescapebuffer-perf branch January 25, 2017 07:25
targos pushed a commit that referenced this pull request Jan 28, 2017
@mscdex @targos
querystring: improve unescapeBuffer performance
a594d18 
PR-URL: #10837
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@mscdex @italoacasas
querystring: improve unescapeBuffer performance
97ff432 
PR-URL: nodejs#10837
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@mscdex @italoacasas
querystring: improve unescapeBuffer performance
99f9f57 
PR-URL: nodejs#10837
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

This will need a backport PR to land on v4. Landed on v6

jasnell pushed a commit that referenced this pull request Mar 8, 2017
@mscdex @jasnell
querystring: improve unescapeBuffer performance
e84efde 
PR-URL: #10837
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@mscdex @MylesBorins
querystring: improve unescapeBuffer performance
db45bf8 
PR-URL: #10837
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins added a commit that referenced this pull request Mar 21, 2017
@MylesBorins
2017-03-21, Version 6.10.1 'Boron' (LTS)
e70d0b0 
Notable changes

* performance: The performance of several APIs has been improved.
  - `Buffer.compare()` is up to 35% faster on average. (Brian White)
    #10927
  - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
    #10895
  - `fs.*statSync()` functions are now up to 9.3% faster on average.
    (Brian White) #11522
  - `os.loadavg` is up to 151% faster. (Brian White)
    #11516
  - `process.memoryUsage()` is up to 34% faster. (Brian White)
    #11497
  - `querystring.unescape()` for `Buffer`s is 15% faster on average.
    (Brian White) #10837
  - `querystring.stringify()` is up to 7.8% faster on average.
    (Brian White) #10852
  - `querystring.parse()` is up to 21% faster on average. (Brian White)
    #10874

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* child_process:
  - `spawnSync` now returns a null `status` when child is terminated by
    a signal. (cjihrig) #11288
  - This fixes the behavior to act like `spawn()` does.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923
  - Debug messages have been added for cases when headers contain
    invalid values. (Evan Lucas)
    #9195

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

* timers:
  - Timer callbacks now always maintain order when interacting with
    domain error handling. (John Barboza)
    #10522

PR-URL: #11759
MylesBorins added a commit that referenced this pull request Mar 21, 2017
@MylesBorins
2017-03-21, Version 6.10.1 'Boron' (LTS)
2569c90 
Notable changes

* performance: The performance of several APIs has been improved.
  - `Buffer.compare()` is up to 35% faster on average. (Brian White)
    #10927
  - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
    #10895
  - `fs.*statSync()` functions are now up to 9.3% faster on average.
    (Brian White) #11522
  - `os.loadavg` is up to 151% faster. (Brian White)
    #11516
  - `process.memoryUsage()` is up to 34% faster. (Brian White)
    #11497
  - `querystring.unescape()` for `Buffer`s is 15% faster on average.
    (Brian White) #10837
  - `querystring.stringify()` is up to 7.8% faster on average.
    (Brian White) #10852
  - `querystring.parse()` is up to 21% faster on average. (Brian White)
    #10874

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* child_process:
  - `spawnSync` now returns a null `status` when child is terminated by
    a signal. (cjihrig) #11288
  - This fixes the behavior to act like `spawn()` does.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923
  - Debug messages have been added for cases when headers contain
    invalid values. (Evan Lucas)
    #9195

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

* timers:
  - Timer callbacks now always maintain order when interacting with
    domain error handling. (John Barboza)
    #10522

PR-URL: #11759
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-03-21, Version 6.10.1 'Boron' (LTS)
ccdb8bf 
    Notable changes

    * performance: The performance of several APIs has been improved.
      - `Buffer.compare()` is up to 35% faster on average. (Brian White)
        nodejs/node#10927
      - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
        nodejs/node#10895
      - `fs.*statSync()` functions are now up to 9.3% faster on average.
        (Brian White) nodejs/node#11522
      - `os.loadavg` is up to 151% faster. (Brian White)
        nodejs/node#11516
      - `process.memoryUsage()` is up to 34% faster. (Brian White)
        nodejs/node#11497
      - `querystring.unescape()` for `Buffer`s is 15% faster on average.
        (Brian White) nodejs/node#10837
      - `querystring.stringify()` is up to 7.8% faster on average.
        (Brian White) nodejs/node#10852
      - `querystring.parse()` is up to 21% faster on average. (Brian White)
        nodejs/node#10874

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * child_process:
      - `spawnSync` now returns a null `status` when child is terminated by
        a signal. (cjihrig) nodejs/node#11288
      - This fixes the behavior to act like `spawn()` does.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923
      - Debug messages have been added for cases when headers contain
        invalid values. (Evan Lucas)
        nodejs/node#9195

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

    * timers:
      - Timer callbacks now always maintain order when interacting with
        domain error handling. (John Barboza)
        nodejs/node#10522

    PR-URL: nodejs/node#11759

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-03-21, Version 6.10.1 'Boron' (LTS)
e47cbbd 
    Notable changes

    * performance: The performance of several APIs has been improved.
      - `Buffer.compare()` is up to 35% faster on average. (Brian White)
        nodejs/node#10927
      - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
        nodejs/node#10895
      - `fs.*statSync()` functions are now up to 9.3% faster on average.
        (Brian White) nodejs/node#11522
      - `os.loadavg` is up to 151% faster. (Brian White)
        nodejs/node#11516
      - `process.memoryUsage()` is up to 34% faster. (Brian White)
        nodejs/node#11497
      - `querystring.unescape()` for `Buffer`s is 15% faster on average.
        (Brian White) nodejs/node#10837
      - `querystring.stringify()` is up to 7.8% faster on average.
        (Brian White) nodejs/node#10852
      - `querystring.parse()` is up to 21% faster on average. (Brian White)
        nodejs/node#10874

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * child_process:
      - `spawnSync` now returns a null `status` when child is terminated by
        a signal. (cjihrig) nodejs/node#11288
      - This fixes the behavior to act like `spawn()` does.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923
      - Debug messages have been added for cases when headers contain
        invalid values. (Evan Lucas)
        nodejs/node#9195

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

    * timers:
      - Timer callbacks now always maintain order when interacting with
        domain error handling. (John Barboza)
        nodejs/node#10522

    PR-URL: nodejs/node#11759

Signed-off-by: Ilkka Myller <[email protected]>
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

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mscdex @Fishrock123 @jasnell @nodejs-github-bot