Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Make QueryString.parse() even faster #2668

Closed
wants to merge 2 commits into from
Closed

Make QueryString.parse() even faster #2668

wants to merge 2 commits into from

Conversation

mscdex
Copy link

@mscdex mscdex commented Feb 2, 2012

Results using @bluesmoon's test query string from #2248:

original qs parse: 9338ms (pre 5166758)
new qs parse: 6436ms (5166758)
newer qs parse: 2898ms (this commit)

qs.split(sep).forEach(function(kvp) {
var x = kvp.split(eq), k, v, useQS=false;
var qssplit = qs.split(sep), regexp = /\+/g;
for (var i=0,len=qssplit.length; i<len; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

Style: needs more spaces.

@bnoordhuis
Copy link
Member

Brian, would it be possible to add a benchmark?

@mscdex
Copy link
Author

mscdex commented Feb 2, 2012

@bnoordhuis here's a gist containing a test for all three implementations: https://gist.github.com/4f7e42c2b49c9a1c99c3

I did notice somewhat slower results with that specific test. My original test was done using 3 separate versions of the querystring module, requiring the parse() function from each one and testing that way.

@isaacs
Copy link

isaacs commented Feb 2, 2012

Let's get this landed in master once we get 0.6.10 merged back. It was probably a mistake to add more performance stuff to 0.6. It'll be nice to brag about 0.8 having a faster querystring.parse :)

Otherwise lgtm.

@isaacs
Copy link

isaacs commented Feb 21, 2012

@mscdex Can you rebase this onto master?

@mscdex
Copy link
Author

mscdex commented Feb 21, 2012

@isaacs Done: #2800

@mscdex mscdex closed this Feb 21, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants