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.js parse(): 1.0241e+5
misc/url.js format(): 1.9642e+5
misc/url.js resolve("../foo/bar?baz=boom"): 23774
misc/url.js resolve("foo/bar"): 43754
misc/url.js resolve("http://nodejs.org"): 33768
misc/url.js resolve("./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.js
   31:22  error  There should be no space before ','  comma-spacing
   32:22  error  There should be no space before ','  comma-spacing
   33:14  error  There should be no space before ','  comma-spacing
   33:28  error  There should be no space before ','  comma-spacing
  228:48  error  There should be no space before ','  comma-spacing
  636:40  error  There should be no space before ','  comma-spacing
  941:48  error  There should be no space before ','  comma-spacing
  950:46  error  There should be no space before ','  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.