querystring: improve querystring unescapeBuffer perf by jseijas · Pull Request #12525 · nodejs/node

jseijas · 2017-04-19T22:40:53Z

Refactored the unescapeBuffer function in order to simplify it,
and also to improve the performance.

The benchmark:

querystring/querystring-unescapebuffer.js n=1000000 input="%20%21%2..."
   18.10 %        *** 6.671620e-29
querystring/querystring-unescapebuffer.js n=1000000 input="there%20..."
   2.75 %          * 1.775320e-02
querystring/querystring-unescapebuffer.js n=1000000 input="there%2Q..."
   34.12 %        *** 8.074612e-25
querystring/querystring-unescapebuffer.js n=1000000 input="there is..."
   27.92 %        *** 4.923348e-29

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)

Refactored the unescapeBuffer function in order to simplify it, and also to improve the performance. The benchmark: ```improvement confidence p.value querystring/querystring-unescapebuffer.js n=1000000 input="%20%21%2..." 18.10 % *** 6.671620e-29 querystring/querystring-unescapebuffer.js n=1000000 input="there%20..." 2.75 % * 1.775320e-02 querystring/querystring-unescapebuffer.js n=1000000 input="there%2Q..." 34.12 % *** 8.074612e-25 querystring/querystring-unescapebuffer.js n=1000000 input="there is..." 27.92 % *** 4.923348e-29 ```

vsemozhetbyt · 2017-04-19T22:58:39Z

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

mscdex · 2017-04-19T22:59:49Z

lib/querystring.js

+  return hasHex ? out.slice(0, outIndex) : out;
 }

-


Unnecessary whitespace change

What is the standard? Because the file contains both 2 and 1 line separation between functions.

My own opinion is that such changes should be addressed separately.

mscdex · 2017-04-19T23:04:36Z

lib/querystring.js

+    if (c === 37 /*'%'*/ && index < maxLength) {
+      c = s.charCodeAt(++index);
+      n = unhexTable[c];
+      if (n < 0) {


This won't work for characters not in the table. This is why I had used !(n >= 0), which will cover undefined as well.

mscdex · 2017-04-19T23:06:02Z

lib/querystring.js

+      } else {
+        c1 = s.charCodeAt(++index);
+        m = unhexTable[c1];
+        if (m < 0) {


mscdex · 2017-04-19T23:07:40Z

The subsystem target in the commit message should be querystring instead of lib.

jseijas · 2017-04-19T23:18:26Z

@mscdex subsystem changed, also the three changes asked commited.

mscdex · 2017-04-19T23:21:20Z

lib/querystring.js

+    if (c === 37 /*'%'*/ && index < maxLength) {
+      c = s.charCodeAt(++index);
+      n = unhexTable[c];
+      if !(n >= 0) {


Missing parens. These sorts of things should be caught by make jslint.

mscdex · 2017-04-19T23:21:27Z

lib/querystring.js

+      } else {
+        c1 = s.charCodeAt(++index);
+        m = unhexTable[c1];
+        if !(m >= 0) {


jseijas · 2017-04-19T23:24:57Z

@mscdex sorry, fixed, lint passing ok, also tests.

mscdex · 2017-04-24T03:01:08Z

lib/querystring.js

-      break;
+  var index = 0;
+  var outIndex = 0;
+  var c, c1, n, m;


Can we name these better and place them on separate lines?

mscdex · 2017-04-24T03:18:29Z

My benchmark results seem to agree more or less with those already posted.

Rename variables and separate in lines.

jseijas · 2017-04-25T22:30:38Z

@mscdex Variables renamed and separated in lines. Lint passing. UT passing. Benchmark times remains similar.

mscdex · 2017-04-25T23:26:54Z

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

jseijas · 2017-05-23T10:50:19Z

Hi @mscdex! Do you need more changes to be made, or you are ok with this? I see that not all the CI is passing :(

lpinca · 2017-05-23T14:23:14Z

SGTM.
New CI: https://ci.nodejs.org/job/node-test-pull-request/8247/

mscdex · 2017-05-23T14:37:43Z

@jseijas It's fine if CI still checks out and there are no performance regressions. Can you please rebase against current master and re-run the benchmarks? I think V8 5.8 was added since this PR was opened and may/may not have an impact on this.

jseijas · 2017-05-23T14:55:03Z

@mscdex Of course I can :) I will come back with the results.

jseijas · 2017-05-23T21:51:16Z

@mscdex Done. Now is even better.

 querystring/querystring-unescapebuffer.js n=1000000 input="%20%21%2..."
    25.54 %        *** 5.020680e-28
 querystring/querystring-unescapebuffer.js n=1000000 input="there%20..."           
    16.14 %        *** 2.536763e-22
 querystring/querystring-unescapebuffer.js n=1000000 input="there%2Q..."                        
    22.93 %        *** 3.068218e-17
 querystring/querystring-unescapebuffer.js n=1000000 input="there is..."                                            
    35.53 %        *** 4.584131e-27

mscdex · 2017-05-24T05:32:04Z

LGTM

lpinca · 2017-05-26T16:42:29Z

Landed in 19685ea.

Refactored the `unescapeBuffer` function in order to simplify it, and also to improve the performance. PR-URL: #12525 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins · 2017-07-17T16:59:43Z

should this land in LTS? If so it will need to bake a bit longer.

Please change labels as appropriate

nodejs-github-bot added the querystring Issues and PRs related to the built-in querystring module. label Apr 19, 2017

vsemozhetbyt added the performance Issues and PRs related to the performance of Node.js. label Apr 19, 2017

mscdex reviewed Apr 19, 2017

View reviewed changes

lib/querystring.js Outdated

} else {

c1 = s.charCodeAt(++index);

m = unhexTable[c1];

if (m < 0) {

Copy link

Contributor

mscdex Apr 19, 2017

Choose a reason for hiding this comment

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

Ditto.

lib: improve querystring unescapeBuffer perf

917ed0a

jseijas changed the title ~~lib: improve querystring unescapeBuffer perf~~ querystring: improve querystring unescapeBuffer perf Apr 19, 2017

mscdex reviewed Apr 19, 2017

View reviewed changes

lib/querystring.js Outdated

} else {

c1 = s.charCodeAt(++index);

m = unhexTable[c1];

if !(m >= 0) {

Copy link

Contributor

mscdex Apr 19, 2017

Choose a reason for hiding this comment

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

Ditto.

querystring: improve querystring unescapeBuffer perf

52c4a67

mscdex reviewed Apr 24, 2017

View reviewed changes

querystring: improve querystring unescapeBuffer perf

5fd16cb

Rename variables and separate in lines.

lpinca approved these changes May 24, 2017

View reviewed changes

lpinca closed this May 26, 2017

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Jul 17, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Uh oh!

Conversation

jseijas commented Apr 19, 2017

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Apr 19, 2017

Uh oh!

jseijas commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseijas commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Apr 24, 2017

Uh oh!

jseijas commented Apr 25, 2017

Uh oh!

mscdex commented Apr 25, 2017

Uh oh!

jseijas commented May 23, 2017

Uh oh!

lpinca commented May 23, 2017

Uh oh!

mscdex commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jseijas commented May 23, 2017

Uh oh!

jseijas commented May 23, 2017

Uh oh!

mscdex commented May 24, 2017

Uh oh!

lpinca commented May 26, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mscdex commented May 23, 2017 •

edited

Loading