url: improve url.parse() performance by mscdex · Pull Request #4892 · nodejs/node · GitHub

This commit improves url.parse() performance by 50-200% with the existing url/url-parse benchmarks. Also, the optimizations made in url.format() result in a 40% increase in performance for url.resolve().

Some optimization strategies used in this commit include:

Combining multiple searches on the same string into a
single loop
Avoiding unnecessary string.split() and array.join()
Minimizing creation of temporary strings
Using a faster alternative to encodeURIComponent,
borrowed from the querystring module

FWIW here is the output from the url-parse benchmark:

url/url-parse.js type=one n=1000000: ./node: 159510 ./node-prev: 88061 .... 81.14%
url/url-parse.js type=two n=1000000: ./node: 185240 ./node-prev: 107190 ... 72.82%
url/url-parse.js type=three n=1000000: ./node: 152400 ./node-prev: 101710 . 49.84%
url/url-parse.js type=four n=1000000: ./node: 685780 ./node-prev: 422830 .. 62.19%
url/url-parse.js type=five n=1000000: ./node: 687790 ./node-prev: 223260 . 208.07%
url/url-parse.js type=six n=1000000: ./node: 171340 ./node-prev: 93834 .... 82.60%

CI: https://ci.nodejs.org/job/node-test-pull-request/1389/

Is this related to #2303?

@silverwind I hadn't seen that PR before this. However, trying that other PR (just url.js actually) locally against master causes test failures with the existing test/parallel/test-url.js.

This PR passes the existing tests and diverges less from the original code (FWIW).

I made the encodeURIComponent() alternative implementation a bit more efficient now. New CI: https://ci.nodejs.org/job/node-test-pull-request/1399/

LGTM if CI is green

Marking this as lts-watch but I definitely think we need to wait to land until after it goes out in a stable for a few weeks.

uh... cc @domenic maybe?

Hrmm, somehow the CI job got lost. Here it is again: https://ci.nodejs.org/job/node-test-pull-request/1463/

CI looks good, but might be worth getting a citgm run on this. /cc @thealphanerd

Just curious: Did var turn out to be faster than the scoped variants?

The resolveObject() changes were mostly unintentional as the url module was modified while I was working on this. However, the reverting of let is intentional as let is still slow in v8. I've left the const as-is though.

This commit improves url.parse() performance by 50-210% with the existing url/url-parse benchmarks. Also, the optimizations made in url.format() result in a 40% increase in performance for url.resolve(). Some optimization strategies used in this commit include: * Combining multiple searches on the same string into a single loop * Avoiding unnecessary string.split() and array.join() * Minimizing creation of temporary strings * Using a faster alternative to encodeURIComponent, borrowed from the querystring module

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/62/

note: run with this patched node https://github.com/TheAlphaNerd/node/tree/improve-url-parse
diff from master: 4897f94...bdef972

Looking at the citgm results the only non-flaky error result is the david module which looks like an unrelated test error. In that particular case the standard module version david is checking for needs to be updated because it's asserting that the standard module is still pre-6.0 when 6.0.0 was release just less than a day ago.

/cc @nodejs/collaborators Any other comments/LGTMs for this?

As far as the other url performance PRs go, I'm not opposed to them if they improve performance even more than this PR. However this PR already passes all tests and is more conservative since it preserves the original url module behavior and is more in line with the original code/design.

LGTM, but I haven't looked at any of the other PRs.

I'm LGTM as well -- I get the impression that the other PR needs more work/review; hopefully will land at a later stage.

In retrospect, we should have landed #2303 much sooner, it's just that we got lost there with those WhatWG tests. Really sorry about that, @STRML.

So, I propose we get this in asap, and possibly backport to LTS.
And we aim for #2303 for node 6 or 7.

These major perf rewrites are not great candidates for LTS cause of their higher risk of finding edge-cases that we won't otherwise find. We improve the chances with letting them bake in v5 for a while but it doesn't make the risk disappear completely.

As it stands I'm -1 on this for v4 unless someone wants to make a case that it's not as major as it looks.

This commit improves url.parse() performance by 50-210% with the existing url/url-parse benchmarks. Also, the optimizations made in url.format() result in a 40% increase in performance for url.resolve(). Some optimization strategies used in this commit include: * Combining multiple searches on the same string into a single loop * Avoiding unnecessary string.split() and array.join() * Minimizing creation of temporary strings * Using a faster alternative to encodeURIComponent, borrowed from the querystring module PR-URL: #4892 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ryan Graham <r.m.graham@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Landed in db91281.

I'm with @rvagg here. I think in general we could be too frisky with backports -- I prefer the more anal "bugfixes only" to avoid corner cases.

performance is a feature

👍 Never know what could be broken months down the line. Glad to see this merged regardless for 5+.

It looks like this was landed three weeks after its last CI run happened. In the intervening time, new lint rules were added. So now make jslint (and CI!) fails on master. It looks like this can be patched up pretty easily/quickly, but I'd want @mscdex to do it if at all possible just in case there are performance implications.

@Trott #5300

This commit improves url.parse() performance by 50-210% with the existing url/url-parse benchmarks. Also, the optimizations made in url.format() result in a 40% increase in performance for url.resolve(). Some optimization strategies used in this commit include: * Combining multiple searches on the same string into a single loop * Avoiding unnecessary string.split() and array.join() * Minimizing creation of temporary strings * Using a faster alternative to encodeURIComponent, borrowed from the querystring module PR-URL: #4892 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ryan Graham <r.m.graham@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Related issue:
#5393

@nodejs/lts this commit introduced some regressions, there are fixes coming in or already in I believe.

Is this something we will want to back port if we have the fixes and it has been stable for a bit?

Part of me want to just say don't land, and avoid potential regressions... but this is some pretty major churn.

@thealphanerd I would delay a while. Same with my timers thing.

@thealphanerd ... I'm going to say hold off for now. There are too many regressions on this. Even with the fixes coming this should sit for a while in master and v5 before we pull it back to v4.

In fact, let's take the lts-watch label off and put the don't land in v4 label on. We can revisit later once we're sure things are stable and the regressions are handled.

I'm a strong -1 on putting this or similar performance commits on to LTS, for simple and clearly low-risk commits we can consider on a case-by-case basis but these introduce too much variability into LTS which we are committed to keeping stable. It's important we achieve and maintain trust with users and give them zero reason to fear upgrading from release to release.

Agree. While I'd love to get performance improvements into LTS in general if possible, these kinds of changes are far too invasive, I think, and open too many possible risks for LTS. Unfortunately, however, not landing them could make cherrypicking more difficult later but we knew that going in.

If this does come it needs to land with #5300

Just noting #5300 has been landed.

@benjamingr I think @thealphanerd meant if it landed on v4.x.

Oh, I saw that the lts-watch has been removed, reading it again it looks like it isn't landing on 4.x either. Sorry.

mscdex added the url Issues and PRs related to the legacy built-in url module. label Jan 27, 2016

mscdex force-pushed the perf-url-parse branch from 5e612b4 to 521927b Compare January 27, 2016 19:29

jasnell added the lts-watch-v4.x label Jan 27, 2016

mscdex force-pushed the perf-url-parse branch 3 times, most recently from 1f2ac71 to c492001 Compare January 28, 2016 18:06

silverwind reviewed Feb 1, 2016
View reviewed changes

mscdex force-pushed the perf-url-parse branch from c492001 to 37c3051 Compare February 1, 2016 18:26

STRML mentioned this pull request Feb 6, 2016

url: Improve url module performance #3 #2303

Closed

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 16, 2016

mscdex closed this Feb 18, 2016

mscdex deleted the perf-url-parse branch February 18, 2016 13:05

Trott mentioned this pull request Feb 18, 2016

test: Lint broken in CI #5299

Closed

orangemocha mentioned this pull request Feb 18, 2016

Enable and require use of node-accept-pull-request? nodejs/build#324

Closed

geek mentioned this pull request Feb 23, 2016

path: performance and stability improvements on all platforms #5123

Closed

Trott mentioned this pull request Mar 1, 2016

url: fix lint and deopt issues #5300

Closed

jasnell added dont-land-on-v4.x and removed lts-watch-v4.x labels Mar 2, 2016

Page- mentioned this pull request Apr 29, 2016

Switch to using hasOwnProperty from the object prototype (nodejs v6) mikenicholson/passport-jwt#59

Merged

url: improve url.parse() performance by mscdex · Pull Request #4892 · nodejs/node · GitHub

Uh oh!

Uh oh!

url: improve url.parse() performance - #4892

url: improve url.parse() performance#4892mscdex wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mscdex:perf-url-parsemscdex/io.js:perf-url-parseCopy head branch name to clipboard

Conversation

mscdex commented Jan 27, 2016

Uh oh!

mscdex commented Jan 27, 2016

Uh oh!

mscdex commented Jan 27, 2016

Uh oh!

silverwind commented Jan 27, 2016

Uh oh!

mscdex commented Jan 27, 2016

Uh oh!

mscdex commented Jan 27, 2016

Uh oh!

jasnell commented Jan 27, 2016

Uh oh!

jasnell commented Jan 27, 2016

Uh oh!

Fishrock123 commented Jan 29, 2016

Uh oh!

mscdex commented Jan 31, 2016

Uh oh!

jasnell commented Feb 1, 2016

Uh oh!

silverwind Feb 1, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Feb 1, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Feb 6, 2016

Uh oh!

mscdex commented Feb 7, 2016

Uh oh!

mscdex commented Feb 17, 2016

Uh oh!

rmg commented Feb 17, 2016

Uh oh!

jbergstroem commented Feb 18, 2016

Uh oh!

silverwind commented Feb 18, 2016

Uh oh!

mcollina commented Feb 18, 2016

Uh oh!

rvagg commented Feb 18, 2016

Uh oh!

mscdex commented Feb 18, 2016

Uh oh!

jbergstroem commented Feb 18, 2016

Uh oh!

petkaantonov commented Feb 18, 2016

Uh oh!

STRML commented Feb 18, 2016

Uh oh!

Trott commented Feb 18, 2016

Uh oh!

mscdex commented Feb 18, 2016

Uh oh!

geek commented Feb 23, 2016

Uh oh!

MylesBorins commented Mar 1, 2016

Uh oh!

Fishrock123 commented Mar 1, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

rvagg commented Mar 3, 2016

Uh oh!

jasnell commented Mar 3, 2016

Uh oh!

MylesBorins commented Mar 17, 2016

Uh oh!

benjamingr commented Mar 23, 2016

Uh oh!

url: improve url.parse() performance#4892
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:perf-url-parse