url: improve url module performance

[petkaantonov]

There is no compatibility breakage and using eager .href is only 33% slower in url parse benchmark, and it could be made lazy again since nobody deletes .href anyway.

misc/url.jsparse(): 1.0241e+5misc/url.jsformat(): 1.9642e+5misc/url.jsresolve("../foo/bar?baz=boom"): 23774misc/url.jsresolve("foo/bar"): 43754misc/url.jsresolve("http://nodejs.org"): 33768misc/url.jsresolve("./foo/bar?baz"): 28106

[Fishrock123]

This is still semver-major, correct?

[petkaantonov]

no there should not be breaking changes

[silverwind]

Let's CI this, also npm tests should pass.

[Fishrock123]

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/650/

[Fishrock123]

make test-npm is happy.

[silverwind]

I'd be fine landing this in a patch release.

@petkaantonov can you outline what's changed from the original implementation?

[petkaantonov]

The parsing has been rewritten (.parse()).

Some minor changes in formatting (.format())

Some micro optimization in resolution (.resolveObject()).

[silverwind]

Needs a rebase because of 19ffb5c

[petkaantonov]

rebased

[silverwind]

Thanks, got a few linter errors concerning this rule: http://eslint.org/docs/rules/comma-spacing

lib/url.js31:22errorThereshouldbenospacebefore ',' comma-spacing32:22errorThereshouldbenospacebefore ',' comma-spacing33:14errorThereshouldbenospacebefore ',' comma-spacing33:28errorThereshouldbenospacebefore ',' comma-spacing228:48errorThereshouldbenospacebefore ',' comma-spacing636:40errorThereshouldbenospacebefore ',' comma-spacing941:48errorThereshouldbenospacebefore ',' comma-spacing950:46errorThereshouldbenospacebefore ',' comma-spacing

[petkaantonov]

Those are not spaces but comments like 0x3A/*':'*/, 0x3F/*'?'*/, ...

[silverwind]

Yeah, it's kind of bullshit that this gets interpreted as a space.

[petkaantonov]

should we open issue in eslint?

[silverwind]

Already been done: eslint/eslint#2408

[silverwind]

Probably best to disable the comma-spacing linter rule until that one is resolved.

[yosuke-furukawa]

How about this ??

const_protocolCharacters=makeAsciiTable([[0x61,0x7A],/*a-z*/[0x41,0x5A],/*A-Z*/0x2E,/*'.'*/0x2B,/*'+'*/0x2D/*'-'*/]);

[silverwind]

@yosuke-furukawa that's just confusing. also doesn't work in cases like

if(!containsCharacter2(search,0x23/*'#'*/,-1))

[yosuke-furukawa]

Hm... I think this is not so weird.

if(!containsCharacter2(search,0x23,/*'#'*/-1))

But +1 for suppress the warnings if you want.

[silverwind]

@yosuke-furukawa would you suggest setting the setting to 1 or 0? Both seem to make make lint pass.

[yosuke-furukawa]

1 is warn, 0 is silence. 1 is better. When we will re-consider the eslint rules, we can detect the rule violation easily.

[silverwind]

@petkaantonov the PR should pass eslint now. I'd advice changing lines like

0x2E/*'.'*/,0x2B/*'+'*/,0x2D/*'-'*/

to

0x2E/*'.'*/,0x2B/*'+'*/,0x2D/*'-'*/

because I assume eslint will still warn on these after the fix.

[petkaantonov]

done

[silverwind]

@petkaantonov thanks. May I ask to add a small test for the delete uri.prop case (#1591)?

[jasnell]

One additional bit to note, the IANA registry for URI schemes lists a few more permanent schemes that require the slash (coap, for instance, http://tools.ietf.org/html/rfc7252) that are currently not listed in the _slashProtocols. Definitely do not need to add those in here but we'll likely want to revisit and add them in later.

[jasnell]

@petkaantonov thank you for persisting with this btw. The improvements look very good on an initial review

[tyscorp]

It'll be great to see this finally land. Great work @petkaantonov

[alubbe]

Hey everybody - what's the status, what's keeping us from merging this? :)

[domenic]

There are still a few outstanding comments, including the request to port the tests from the previous PR. Plus, it needs to be rebased, since it no longer merges cleanly.

[petkaantonov]

I am really lacking the time at the moment

[Fishrock123]

reference to original: #1561

[alubbe]

No worries @petkaantonov
I was just checking in because Techempower is gearing up to run the next benchmark suite and I remember the low node numbers being part of your motivation to work on this in the first place.
We also landed significant speed ups for the express+jade app, so the next round should yield pretty good numbers.

[ronkorving]

Out of interest, is this still going places?

[jbergstroem]

@ronkorving it nowadays live in #2303.

[ronkorving]

Ah thanks, better close this one then I guess?

[jbergstroem]

I think its left open in case petka has time to resume.

[benjamingr]

Going to cloes this, Petka said he's likely not going to resume, the changes live in another PR and this can be reopend anyway if anyone disagrees.