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

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex 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
Copy link
Contributor

Avoid caching result of typeof.

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

@mscdex
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
Copy link
Contributor

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
Copy link
Contributor

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
Double negation considered slower than a straight null check.
brendanashworth pushed a commit that referenced this pull request Feb 15, 2015
PR-URL: #847
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@mscdex
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
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

if (str.length === 0)
return str;

var len = str.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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.
@trevnorris
Copy link
Contributor

I'm cool with this.

@Fishrock123 Fishrock123 added the querystring Issues and PRs related to the built-in querystring module. label Feb 23, 2015
@mscdex mscdex mentioned this pull request Feb 24, 2015
@mscdex
Copy link
Contributor Author

mscdex commented Mar 4, 2015

Everything look alright for merging this?

/cc @iojs/collaborators

@cjihrig
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
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
Copy link
Contributor Author

mscdex commented Mar 16, 2015

Landed in 85a92a3.

@mscdex mscdex closed this Mar 16, 2015
@mscdex mscdex deleted the perf-querystring branch March 16, 2015 03:16
@rvagg rvagg mentioned this pull request Mar 17, 2015
@AdriVanHoudt AdriVanHoudt mentioned this pull request Mar 19, 2015
@rvagg rvagg mentioned this pull request Mar 20, 2015
rvagg added a commit that referenced this pull request Mar 20, 2015
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
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
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
Labels
querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants