url: significantly improve the performance of the url module

[petkaantonov]

See issues: #643, nodejs/node-v0.x-archive#6788

Original message:

I have rewritten the url parser module of node core as it was/is a serious bottleneck in some of the techempower benchmarks

Running node's urlparser benchmark using iojs it's still 16x faster when not retrieving properties and 11x faster when retrieving all properties (which are lazy getters in my implementation). In absolute terms the current iojs urlparser throughputs 25k parses per second vs 400k lazy/270k eager parses per second. format and resolve are also affected in similar magnitudes.

Testing this PR on my VM ubuntu I get

After

$ bleed benchmark/misc/url.js
misc/url.js parse(): 1.6529e+5
misc/url.js format(): 1.9114e+5
misc/url.js resolve("../foo/bar?baz=boom"): 24302
misc/url.js resolve("foo/bar"): 35696
misc/url.js resolve("http://nodejs.org"): 68003
misc/url.js resolve("./foo/bar?baz"): 29983

$ bleed benchmark/common.js url
url/url-parse.js
url/url-parse.js type=one n=250000: 1244025.74054
url/url-parse.js type=two n=250000: 2071492.21773
url/url-parse.js type=three n=250000: 1113916.06067
url/url-parse.js type=four n=250000: 3416688.31936
url/url-parse.js type=five n=250000: 1620766.16897
url/url-parse.js type=six n=250000: 1659564.59318

url/url-resolve.js
url/url-resolve.js type=one n=100000: 184353.89591

Before

$ git checkout v1.x && make -j4 && bleed benchmark/misc/url.js
misc/url.js parse(): 16510
misc/url.js format(): 18309
misc/url.js resolve("../foo/bar?baz=boom"): 8929.9
misc/url.js resolve("foo/bar"): 9919.5
misc/url.js resolve("http://nodejs.org"): 8177.6
misc/url.js resolve("./foo/bar?baz"): 8984.4

$ bleed benchmark/common.js url
url/url-parse.js
url/url-parse.js type=one n=250000: 82666.55318
url/url-parse.js type=two n=250000: 84669.54973
url/url-parse.js type=three n=250000: 80308.64406
url/url-parse.js type=four n=250000: 262017.65403
url/url-parse.js type=five n=250000: 203834.95043
url/url-parse.js type=six n=250000: 68911.18834

url/url-resolve.js
url/url-resolve.js type=one n=100000: 42331.42854

That's 10-24x faster depending on url.

Caveats

This implementation uses getters/setters on the prototype instead of own properties like current url. This is because often the properties are not needed and take time to calculate. Additionally @chrisdickinson introduced another reason to use setters at #893 (comment).

This patch is technically backward incompatible but to minimize any compatibility issues I made util._extend to call toJSON, because some core modules pass the Url instance directly to util._extend. The toJSON also means that serialized output of current url and the new url stay the same.

/cc @trevnorris @mikeal

[benjamingr]

I've been using Petka's url replacement in my apps for a while now and generally it works well. This can potentially speed up a lot of apps for free. The caveats don't sound too bad either. I admit that the fact this was not merged into node (or at least reviewed) when it was first written was quite a surprise for me.

[domenic]

Is this new implementation more or less compliant with the URL Standard? How does it stack up on the URL test suite?

[petkaantonov]

@domenic The goal was not to make a new url parser, just to optimize the existing one. So it should give exactly the same result as the current url url parser - if that means some will fail there then so be it.

[mikeal]

perhaps we should take this in the next major semver bump?

[petkaantonov]

@mikeal that's ok with me, what branch do I pr against for major?

[mikeal]

@petkaantonov we're still figuring that out :) we have the same problem with a PR for improving util.isObject and didn't entirely resolve it in the last TC meeting. I'll add this one to the agenda though.

[domenic]

I know we don't have any data (and @mikeal really wants a way to make some) but I'd be surprised if this was actually back-compat-breaking in any way.

[mikeal]

@domenic this thread is pretty long but I seem to remember that there is an issue with the return value not working util.extends? That will surely cause breaks in a few people's applications.

[domenic]

@mikeal ah yeah you're right same with xtend. I take it back.

[petkaantonov]

@mikeal util._extends is changed to call toJSON() - many internals pass Url object directly to util extend as well. toJSON is not an arbitrary method name, it is used by JSON.stringify as well.

https://github.com/iojs/io.js/pull/933/files#diff-3deb3f32958bb937ae05c6f3e4abbdf5R635

[mikeal]

@petkaantonov are you saying it is changed or that it would need to change?

[petkaantonov]

@mikeal It is changed in this PR as I linked

[mikeal]

@petkaantonov sorry, in my email that link wasn't there :)

[petkaantonov]

Yea I have a really bad habit of firing comments asap and then editing them later :P

[mikeal]

Is there a perf impact elsewhere from changing ._extends?

[domenic]

I am not sure that changing _extend's semantics is a very good idea. It'd be better to change usages of it with URLs throughout the codebase to instead call toJSON() before passing to _extends.

[mikeal]

@domenic what do you mean by "throughout the codebase?" the real issue isn't usage in core but usage by applications.

[domenic]

I'm talking purely about _extends here. I agree that in general people will use xtend or similar tools with URLs and this change will break that, which means overall its semver-major. I just am nitpicking @petkaantonov's particular mitigating strategy for handling core.

[petkaantonov]

Is there a perf impact elsewhere from changing ._extends?

I cannot find a benchmark but I would be surprised if a property lookup and type-check would even show up considering the code on the very next line loads all properties of the object in an array.

[petkaantonov]

@domenic I did it this way because util._extends is essentially used as public method I think.

[domenic]

@petkaantonov yes and I think modifying its behavior in the face of toJSON-able objects is not a good change to make.

[petkaantonov]

Well it could always be a hardcoded check for Url instance as well, but I was thinking that if you really defined a toJSON method on an object, you would prefer to use the properties returned by that when using ._extend.

[rvagg]

@petkaantonov what's the status of this? Are you OK with @domenic's suggestion of putting getters & setters on all properties for consistency? If so, can we go ahead and get this merged or is there something else holding it up?

[petkaantonov]

@rvagg Yes I am OK with it. I just haven't gotten around to do the work.

[rvagg]

@petkaantonov cool, no problems, just let us know if timing sucks too much and we can cut it from 2.0.0 to take the pressure off.

[petkaantonov]

Ill work on this today and see if I can finish this

[petkaantonov]

Making everything accessors has given some unexpected trouble with the resolveObject method, it's gonna take one more day or two

[petkaantonov]

Moved to #1561 which is rebased against master so that it's possible to review it