Revert url perf changes

[rvagg]

For discussion purposes at the moment, continuing from #1591, but this is the easiest path that I see to getting a less controversial 2.0.0 out.

[mikeal]

+1

BTW, if someone wants to put this behind a flag they can do that in a minor release since it would only be considered a feature addition.

[jcrugzz]

I'm +1 here as well. I know there is more than a handful of possible places in my own modules or modules I maintain where this may result in a subtle breakage (including node-http-proxy possibly).

[bnoordhuis]

LGTM. Let's not hold up the release.

[othiym23]

Wearing my npm maintainer's tiara, I'm +1 on this if the goal is to get [email protected] out any sooner than 10 days from now, which is the earliest date at which an npm build I'm going to feel is safe to use with this code will be available.

[chrisdickinson]

+1. Let's get this in behind a flag (or as @mikeal suggests, published on npm as a -r preload module) in a minor after v2.0.0 so we can start paving the way for this to land.

[isaacs]

+1. I apologize for not weighing in on #1561 initially.

I'd like to propose a rule that any commit message that mentions performance MUST include either specific data on the improvement of a pre-existing benchmark, or a new benchmark that shows improvement with the patch. If others here agree this is an idea worth pursuing, I'll send a PR for the contribution guide.

Without specific and reproducible benchmark results, it's impossible to even have a discussion about performance. Even if there are improved benchmarks, the benefit has to be weighed against the costs, including the costs of a lack of stability, and must be put in perspective of how big a performance difference it'll make to a real application. What's hurting me much more these days is child_process performance, not url parsing. Even before this change, we could parse 1,000,000 urls in just a few seconds.

If we do re-visit #1561, it should also be split into separate bits for the benefit of good atomic commit hygiene. Switching from object properties to accessors is a significant change. There are other changes to the parsing approach, renamed variables, and so on.

Just to try to put some level of data behind what the performance impact is here, I came up with this very simple benchmark:

# master today, with the "performance enhancements"
$ for i in 1 2 3 4 5; do node -p 'var url=require("url");var s=process.hrtime();for(var i=0;i<100000;i++)url.parse("http://a:[email protected]/asdf?q=1#hash");s=process.hrtime(s);100000/(s[0]+s[1]/1e9)'; done
156803.7345029713
152023.75643620887
150275.67862858463
151732.90939077525
150361.49225823936
# average = 152239.51424335586 url parses per second

# this patch, performance degraded
$ for i in 1 2 3 4 5; do node -p 'var url=require("url");var s=process.hrtime();for(var i=0;i<100000;i++)url.parse("http://a:[email protected]/asdf?q=1#hash");s=process.hrtime(s);100000/(s[0]+s[1]/1e9)'; done
159153.7304684274
156878.3701837095
156537.3031852965
145164.64711456324
148571.17474394385
# average = 153261.04513918812 url parses per second

There is no performance difference to be had. Even if there was, it's an operation that can already be performed in excess of 150k times per second, and no one has that many urls to parse.

Therefor, the only consideration should be whether or not the semantic changes on their own are worth the disruption. I think it's clear that they're not. It would be better if this module were taken to userland, and leave url.js as it was.

[chrisdickinson]

The bench results were attached to the original PR, and didn't get ported over to the final PR (which was cut because github doesn't allow PRs to be retargeted). Sorry for the information loss (which is due to me switching the dev branch from v1.x to master)!

[Fishrock123]

+1

I wouldn't mine it flagged or as a preload module, so long as that takes us basically no time to do.

[rvagg]

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

will merge if we get a 👍 from CI

[rvagg]

merged f34b105

[rvagg]

Really sorry about @petkaantonov, I was looking forward to getting this out there but I'm also glad that we're taking a more safe approach to compatibility with the ecosystem; hopefully we can find a path forward that gets this back in sooner than later. Thanks for the excellent work on this so far!

[rvagg]

forgot to put metadata in all 3 commits, done and force pushed as 0f39ef4, 0daed24 and 702997c

[petkaantonov]

@isaacs The performance improvement is up to 25x, but at least 10x as shown in many issue/pr threads linked in the real PR. The absolute speed is only 10k parses per second, so it is a major bottleneck for web servers since express parses urls per each request. It improves TechEmpower single query benchmark score by 30% (yes, that benchmark queries a database as well as renders templates, that's how slow url parsing is).

The reason for making everything an accessor is so that 'url' can be made work just like it works in browsers, not for performance. Currently it works nothing like in browsers, although it does do some horrible things just because browsers do it, appending ':' to protocols for instance. For performance you only need to make .href an accessor (and even this can be debated, please see the real PR).

This has been in userland for a long time (without everything being an accessor though) now but as far as I can tell from anecdotal evidence, many people aren't using io.js simply because there is nothing really substantial over node. 30% faster web servers would have surely been such a thing.

[YurySolovyov]

Maybe current parser could be re-written in several passes, eliminating bottlenecks one-by-one?
BTW, where current "slowness" comes form?

[spion]

@isaacs I don't know what you've been testing, but here is what I get:

% for i in 1 2 3 4 5; do node -p 'var url=require("url");var s=process.hrtime();for(var i=0;i<100000;i++)url.parse("http://a:[email protected]/asdf?q=1#hash");s=process.hrtime(s);100000/(s[0]+s[1]/1e9)'; done
144431.19737550025
156379.18875731213
158876.53319329955
158296.33656418743
153327.86955603285

# petkaanotnov's url extracted to a separate file called url2.js
5838 % for i in 1 2 3 4 5; do node -p 'var url=require("./url2");var s=process.hrtime();for(var i=0;i<100000;i++)url.parse("http://a:[email protected]/asdf?q=1#hash");s=process.hrtime(s);100000/(s[0]+s[1]/1e9)'; done
1874383.4332845176
1943702.0690358658
1811787.4930533804
1794612.6159974784
1942542.0913905445

That is a 12x performance gain. Can you double-check if you actually tested the real thing?

edit: thats not even enough iterations for the JIT warmup to be taken into account, the actual number for the faster url parser is about 2.3 million, making it a 15x improvement for that particular test case

[Fishrock123]

To be clear here:

I'd like to see it land in the future, but we need to do some prep on the ecosystem so that important things like npm actually still work. :)

[silverwind]

Please move the discussion to #643.