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: optimize parse and stringify #847

Closed
wants to merge 1 commit into
base: v1.x
from

Conversation

Projects
None yet
7 participants
@mscdex
Copy link
Contributor

mscdex commented Feb 14, 2015

parse optimizations:

  • Move try-catch to separate function to keep entire function from
    being deoptimized.
  • Use key array lookup instead of using hasOwnProperty.
  • Avoid decoding known empty strings.
  • Avoid possibly unnecessary switch to slower decoder for values if
    key decoding throws.

stringify optimizations:

  • Use manual loop for default encoder instead of encodeURIComponent.
  • Use string concatenation instead of joining an array of strings.
  • Avoid caching result of typeof.

Results with benchmarks from #846:

querystring/querystring-parse.js type=noencode n=1000000: ./iojs-after: 296290 ./iojs-before: 224220 ........ 32.14%
querystring/querystring-parse.js type=encodemany n=1000000: ./iojs-after: 263410 ./iojs-before: 213780 ...... 23.22%
querystring/querystring-parse.js type=encodelast n=1000000: ./iojs-after: 284420 ./iojs-before: 194230 ...... 46.44%
querystring/querystring-stringify.js type=noencode n=1000000: ./iojs-after: 983630 ./iojs-before: 610680 .... 61.07%
querystring/querystring-stringify.js type=encodemany n=1000000: ./iojs-after: 857700 ./iojs-before: 187150 . 358.30%
querystring/querystring-stringify.js type=encodelast n=1000000: ./iojs-after: 987650 ./iojs-before: 290730 . 239.72%
@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Feb 14, 2015

Avoid caching result of typeof.

Do you have a link to something that explains this one?

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Feb 14, 2015

@vkurchatkin No particular link. I saw someone mention it in one of the other PRs and thought I'd try it out. It's not a huge performance boost, but there was a difference.

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Feb 14, 2015

My general rule is make a var where there are are 3-4+ references to like a typeof, or a thing.thing.value_you_want. Then it makes sense for clarity.

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Feb 14, 2015

Ok, after looking through v8 I found some hints that typeof x === "y" is compiled as a whole

Fishrock123 referenced this pull request in ceejbot/io.js Feb 14, 2015

Perf tweak as suggested by @cjihrig
Double negation considered slower than a straight null check.

brendanashworth added a commit that referenced this pull request Feb 15, 2015

benchmark: add a few querystring benchmarks
PR-URL: #847
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Feb 16, 2015

Don't merge just yet, I think I may have some further improvements at least for parse...

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Feb 17, 2015

Ok, scratch that. For some reason I see a larger increase with my uncommitted changes to parse() than the current committed changes with benchmark.js-based benchmarks but not so with the io.js querystring benchmarks (same inputs). I'm ok with this current implementation for now though.

Thoughts?

/cc @bnoordhuis @trevnorris

@rvagg rvagg referenced this pull request Feb 18, 2015

Closed

Release proposal: v1.3.0 #872

@mscdex mscdex force-pushed the mscdex:perf-querystring branch Feb 18, 2015

@brendanashworth

View changes

lib/querystring.js Outdated
if (str.length === 0)
return str;

var len = str.length;

This comment has been minimized.

@brendanashworth

brendanashworth Feb 19, 2015

Member

would it be worth it to bump this line up before the str.length === 0 check?

querystring: optimize parse and stringify
parse optimizations:

* Move try-catch to separate function to keep entire function from
being deoptimized.
* Use key array lookup instead of using hasOwnProperty.
* Avoid decoding known empty strings.
* Avoid possibly unnecessary switch to slower decoder for values if
key decoding throws.

stringify optimizations:

* Use manual loop for default encoder instead of encodeURIComponent.
* Use string concatenation instead of joining an array of strings.
* Avoid caching result of typeof.

@mscdex mscdex force-pushed the mscdex:perf-querystring branch to 6610767 Feb 19, 2015

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Feb 20, 2015

I'm cool with this.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 4, 2015

Everything look alright for merging this?

/cc @iojs/collaborators

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Mar 5, 2015

We should see what the CI says before merging.

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/245/

mscdex added a commit to mscdex/io.js that referenced this pull request Mar 16, 2015

querystring: optimize parse and stringify
parse optimizations:

* Move try-catch to separate function to keep entire function from
being deoptimized.
* Use key array lookup instead of using hasOwnProperty.
* Avoid decoding known empty strings.
* Avoid possibly unnecessary switch to slower decoder for values if
key decoding throws.

stringify optimizations:

* Use manual loop for default encoder instead of encodeURIComponent.
* Use string concatenation instead of joining an array of strings.
* Avoid caching result of typeof.

PR-URL: nodejs#847
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 16, 2015

Landed in 85a92a3.

@mscdex mscdex closed this Mar 16, 2015

@mscdex mscdex deleted the mscdex:perf-querystring branch Mar 16, 2015

@rvagg rvagg referenced this pull request Mar 17, 2015

Closed

Release proposal: v1.6.0 #1157

@AdriVanHoudt AdriVanHoudt referenced this pull request Mar 19, 2015

Closed

Perf boost #77

@rvagg rvagg referenced this pull request Mar 20, 2015

Closed

Release proposal: v1.6.1 #1212

rvagg added a commit that referenced this pull request Mar 20, 2015

2015-03-20 io.js v1.6.1 Release
Notable Changes:

* path: New type-checking on path.resolve()
  <#1153> uncovered some edge-cases
  being relied upon in the wild, most notably path.dirname(undefined).
  Type-checking has been loosened for path.dirname(), path.basename(),
  and path.extname(), (Colin Ihrig)
  <#1216>.
* querystring: Internal optimizations in querystring.parse() and
  querystring.stringify() <#847>
  prevented Number literals from being properly converted via
  querystring.escape() <#1208>,
  exposing a blind-spot in the test suite. The bug and the tests have
  now been fixed (Jeremiah Senkpiel)
  <#1213>.
@narqo

This comment has been minimized.

Copy link

narqo commented Mar 29, 2015

Ok, after looking through v8 I found some hints that typeof x === "y" is compiled as a whole

@vkurchatkin I'd be grateful if you could point me on such hints. It sound really interesting to me, but I couldn't find any confirmations.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 29, 2015

@narqo If you're looking for the v8 commit that added that optimization, it is r9688.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment