-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Conversation
Thank you! Looks really promising. Could you please try running our benchmarks with and without your change?
Please submit the results here. |
Can compare.js be used to compare changes in the libs only? The node binaries should not actually change in this case. |
@gwicke unfortunately, no. But building binaries is easy, and you'll need to do it anyway in order to run tests. |
@gwicke also the binaries do change, because js files are embedded into them |
@indutny ah, makes sense. Running now after some futzing with the parameters. |
Also submitted similar code to node core in nodejs/node-v0.x-archive#7878
The results look fairly uninteresting. Slightly over 3% is the biggest change I see in any test. I suspect that url parsing is not actually tested, as it's not part of the http module? |
Oh, sorry! That's right... |
The question is - how it affects other cases in |
@indutny : The effect on other uses of This is not that surprising, as only one conditional and a simple regexp test are added. The regexp fails quickly if the first char is not a slash. |
Here's another run using benchmark.js for the slow path ('http://example.com/'), best of 500 runs, 30 second test time:
|
This is the benchmark code: var url = require('./url.js');
var Benchmark = require('benchmark');
var bench = new Benchmark(function() {
return url.parse('http://example.com/bar');
},
{
minSamples: 2000
});
var samples = bench.run().stats.sample.sort();
var perc25 = samples[Math.ceil(samples.length / 4)]
console.log('slow path', perc25); |
LGTM, @trevnorris your +1? |
I would make the following changes: diff --git a/lib/url.js b/lib/url.js
index fcf40c8..f0be09c 100644
--- a/lib/url.js
+++ b/lib/url.js
@@ -52,7 +52,7 @@ var protocolPattern = /^([a-z0-9.+-]+:)/i,
portPattern = /:[0-9]*$/,
// Special case for a simple path URL
- simplePathPattern = /^(\/(?!\/)[^\?#\s]*)(\?[^#\s]*)?$/,
+ simplePathPattern = /^(\/\/?(?!\/)[^\?\s]*)(\?[^\s]*)?$/,
// RFC 2396: characters reserved for delimiting URLs.
// We actually just auto-escape these.
@@ -122,7 +122,7 @@ Url.prototype.parse = function(url, parseQueryString, slashesDenoteHost) {
// This is to support parse stuff like " http://foo.com \n"
rest = rest.trim();
- if (!slashesDenoteHost) {
+ if (!slashesDenoteHost && hashSplit.length === 1) {
// Try fast path regexp
var simplePath = simplePathPattern.exec(rest);
if (simplePath) { This allows for the simple case that starts with Other than that it looks good to me. |
This patch adds a fast path for parsing of simple path-only URLs, as commonly found in HTTP requests received by a server. Benchmark results [ms], before / after patch: /foo/bar 0.008956 0.000418 (fast path used) http://example.com/ 0.011426 0.011437 (normal slow path, no change) In a simple 'ab' benchmark of a single-threaded web server, this patch increases the request rate from around 6400 to 7400 req/s.
@trevnorris : Pushed new patch with your suggested changes. I was also considering to add support for simple lower-cased https? protocol urls, but that's slightly more complex and can wait for another patch. |
Cool. LGTM. I'll merge when I'm near a computer. |
Ping! |
FWIW, |
Landed in 4b59db0, thank you! Sorry for a delay! |
Ah crap, yeah sorry about that. Thanks @indutny for the follow up. |
Thanks @indutny & no worries @trevnorris ! |
if (simplePath[2]) { | ||
this.search = simplePath[2]; | ||
if (parseQueryString) { | ||
this.query = querystring.parse(this.search); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gwicke, I just found an error that I think it's caused by this line.
It seems that in te previous version, the parse function was called after removing the "?" from the this.search part, but now it's included, so the first querystring parameter has a "?" prepended.
Just changing this to this.query = querystring.parse(this.search.substr(1));
should fix this, i think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give a before/after example? Just curious, and also it should be added as a test. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @trevnorris,
➜ node git:(master) node -v
v0.10.26
➜ node git:(master) node
> require('url').parse('/profile?access_token=1', true);
{ protocol: null,
slashes: null,
auth: null,
host: null,
port: null,
hostname: null,
hash: null,
search: '?access_token=1',
query: { access_token: '1' },
pathname: '/profile',
path: '/profile?access_token=1',
href: '/profile?access_token=1' }
➜ node git:(master) ./node -v
v0.13.0-pre
➜ node git:(master) ./node
> require('url').parse('/profile?access_token=1', true);
{ protocol: null,
slashes: null,
auth: null,
host: null,
port: null,
hostname: null,
hash: null,
search: '?access_token=1',
query: { '?access_token': '1' },
pathname: '/profile',
path: '/profile?access_token=1',
href: '/profile?access_token=1' }
Look the query
property in the parsed uri.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just made a PR for this, #8151
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@warseph: Thanks for the fix!
This patch adds a fast path for parsing of simple path-only URLs, as commonly
found in HTTP requests received by a server.
Benchmark results [ms], before / after patch:
In a simple ab benchmark of a single-threaded web server, this patch
increases the request rate from around 6400 to 7400 req/s.