querystring: improve parse() performance#4675

mscdex · 2016-01-13T19:53:30Z

These changes improve parse() performance from ~12-32% on all of the existing querystring benchmarks:

querystring/querystring-parse.js type=noencode n=1000000: ./node: 444400 ./node-prev: 369170 ... 20.38% querystring/querystring-parse.js type=encodemany n=1000000: ./node: 383030 ./node-prev: 288910 . 32.58% querystring/querystring-parse.js type=encodelast n=1000000: ./node: 392020 ./node-prev: 336360 . 16.55% querystring/querystring-parse.js type=multivalue n=1000000: ./node: 269660 ./node-prev: 240230 . 12.25%

mscdex · 2016-01-13T19:55:08Z

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

evanlucas · 2016-01-13T19:55:22Z

lib/querystring.js

Is instanceof reliable for arrays?

It should be and we're the only one creating the values in this function, so it's safe.

mscdex · 2016-01-13T20:43:23Z

CI is green except for unrelated CI/test failures on Windows.

jasnell · 2016-01-13T20:46:55Z

LGTM

jbergstroem · 2016-01-13T22:26:41Z

LGTM

geek · 2016-01-13T22:38:12Z

lib/querystring.js

is r the result and s the place to slice? Either way, please make the variables clearer. I know the code above isn't doing any better with k and v :/

rvagg · 2016-01-14T02:09:51Z

I'd like us to get in the habit of placing comments in code that exists in core specifically for performance reasons. The replacement of a regex replace() with a 15 line function, like so many similar hacks we have in place, is like a landmine to new contributors who don't have the history. A comment about why it's done this way would also serve as a flag for future contributors to have a look to see if it's still true that a replace() is 20% slower than manually parsing the string because the perf profile will change over time.

i.e. a comment either above the new function or at the place it's used about why it's there would be nice please!

ronkorving · 2016-01-14T11:29:50Z

lib/querystring.js

Have you tried moving the regexp out of the function so it doesn't keep getting initialized over and over?

There's very little difference in performance. v8 is pretty good at regexps, especially simple ones like /\+/.

mscdex · 2016-01-14T18:44:57Z

@rvagg Done.

These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks.

cjihrig · 2016-01-14T18:53:15Z

LGTM with the added comments

These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks. PR-URL: #4675 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2016-01-15T02:03:52Z

Landed in 4bc1a47

jasnell · 2016-01-15T02:05:15Z

I'm pulling the lts-watch off this since the benchmarks were run against v4.7 and it's not clear yet if the same impact would be had on v4.

mscdex · 2016-01-15T02:25:27Z

@jasnell I just tested the changes on the v4.x branch and I saw similar benchmark results. For example:

querystring/querystring-parse.js type=noencode n=1000000: ./node: 442340 ./node-prev-4: 362100 ... 22.16% querystring/querystring-parse.js type=encodemany n=1000000: ./node: 385380 ./node-prev-4: 339430 . 13.54% querystring/querystring-parse.js type=encodelast n=1000000: ./node: 439090 ./node-prev-4: 363250 . 20.88% querystring/querystring-parse.js type=multivalue n=1000000: ./node: 284120 ./node-prev-4: 250630 . 13.36%

jasnell · 2016-01-15T02:27:00Z

Awesome. OK, that's better then. :)

These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks. PR-URL: #4675 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable changes: * events: make sure console functions exist (Dave) #4479 * fs: add autoClose option to fs.createWriteStream (Saquib) #3679 * http: improves expect header handling (Daniel Sellers) #4501 * node: allow preload modules with -i (Evan Lucas) #4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) #4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622 - module: cache stat() results more aggressively (Ben Noordhuis) #4575 - querystring: improve parse() performance (Brian White) #4675 PR-URL: #4742

These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks. PR-URL: #4675 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks. PR-URL: nodejs#4675 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks. PR-URL: #4675 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable changes: This update to the LTS line includes a number of semver minor changes that have been staged for a number of months. This includes: * deps: backport 9da3ab6 from V8 upstream (Ali Ijaz Sheikh) - #3609 * http: handle errors on idle sockets (José F. Romaniello) - #4482 * src: add BE support to StringBytes::Encode() (Bryon Leung) - #3410 * tls: add `options` argument to createSecurePair (Коренберг Марк) - #2441 There are also quite a large number of semver patch changes including over 20 doc fixes and almost 50 test fixes. Notable semver patch changes include: * deps: upgrade to npm 2.14.18 (Kat Marchán) - #5245 * https: evict cached sessions on error (Fedor Indutny) - #4982 * process: support symbol events (cjihrig) - #4798 * querystring: improve parse() performance (Brian White) - #4675 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. Notable changes: The SEMVER-MINOR changes include: * **deps**: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * **http**: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * **src**: - Better support for Big-Endian systems (Bryon Leung) #3410 * **tls**: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 Notable semver patch changes include: * **npm** - upgrade to npm 2.14.19 (Kat Marchán) #5335 * **https**: - A potential fix for #3692) HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * **process**: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * **querystring**: - querystring.parse() is now 13-22% faster! (Brian White) #4675 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) #5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) nodejs#3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) nodejs#4482 * src: - Better support for Big-Endian systems (Bryon Leung) nodejs#3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) nodejs#2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) nodejs#4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) nodejs#4841 * https: - A potential fix for nodejs#3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) nodejs#4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) nodejs#3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) nodejs#5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) nodejs#5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) nodejs#4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) nodejs#4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) nodejs#4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) nodejs#5214 PR-URL: nodejs#5301

These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks. PR-URL: #4675 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301

These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks. PR-URL: nodejs#4675 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable changes: * events: make sure console functions exist (Dave) nodejs#4479 * fs: add autoClose option to fs.createWriteStream (Saquib) nodejs#3679 * http: improves expect header handling (Daniel Sellers) nodejs#4501 * node: allow preload modules with -i (Evan Lucas) nodejs#4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) nodejs#4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - module: cache stat() results more aggressively (Ben Noordhuis) nodejs#4575 - querystring: improve parse() performance (Brian White) nodejs#4675 PR-URL: nodejs#4742

mscdex added the querystring Issues and PRs related to the built-in querystring module. label Jan 13, 2016

evanlucas reviewed Jan 13, 2016
View reviewed changes

geek reviewed Jan 13, 2016
View reviewed changes

jasnell added the lts-watch-v4.x label Jan 14, 2016

mscdex force-pushed the querystring-parse-perf branch from 33b48b4 to 202af5cCompare January 14, 2016 01:11

ronkorving reviewed Jan 14, 2016
View reviewed changes

mscdex force-pushed the querystring-parse-perf branch from 202af5c to 597ae96Compare January 14, 2016 18:44

mscdex force-pushed the querystring-parse-perf branch from 597ae96 to 8f3675dCompare January 14, 2016 18:47

querystring: improve parse() performance
ca9b4bb
These changes improve parse() performance from ~11-30% on all of the existing querystring benchmarks.

mscdex force-pushed the querystring-parse-perf branch from 8f3675d to ca9b4bbCompare January 14, 2016 18:49

jasnell closed this Jan 15, 2016

jasnell removed the lts-watch-v4.x label Jan 15, 2016

mscdex added the lts-watch-v4.x label Jan 15, 2016

mscdex deleted the querystring-parse-perf branch January 15, 2016 04:59

evanlucas mentioned this pull request Jan 18, 2016
v5.5.0 Release Proposal (Stable) #4742
Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 28, 2016

MylesBorins mentioned this pull request Feb 11, 2016
V4.3.1 proposal #5200
Merged

MylesBorins mentioned this pull request Feb 18, 2016
V4.4.0 proposal #5301
Merged

jasnell mentioned this pull request Apr 25, 2017
Collaborator nominations #12646
Closed

Uh oh!

querystring: improve parse() performance#4675

querystring: improve parse() performance #4675

Uh oh!

Conversation

mscdex commented Jan 13, 2016

Uh oh!

mscdex commented Jan 13, 2016

Uh oh!

evanlucasJan 13, 2016

Choose a reason for hiding this comment

Uh oh!

mscdexJan 13, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jan 13, 2016

Uh oh!

jasnell commented Jan 13, 2016

Uh oh!

jbergstroem commented Jan 13, 2016

Uh oh!

geekJan 13, 2016

Choose a reason for hiding this comment

Uh oh!

rvagg commented Jan 14, 2016

Uh oh!

ronkorvingJan 14, 2016

Choose a reason for hiding this comment

Uh oh!

mscdexJan 14, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jan 14, 2016

Uh oh!

cjihrig commented Jan 14, 2016

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

mscdex commented Jan 15, 2016

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants