url: performance improvement in URL implementation #10469

jasnell · 2016-12-27T05:01:30Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

url

Description of change

Yields about a 25% average performance improvement

mscdex · 2016-12-27T05:17:12Z

It looks like the file mode was changed accidentally.

mscdex · 2016-12-27T05:19:55Z

I've added dont-land-on-* labels because of the use of bind() which is only faster in node v7.

jasnell · 2016-12-27T05:23:51Z

grr.. ok, file mode should be fixed now.

Trott · 2016-12-27T05:47:00Z

lib/internal/url.js

+                                 host, port, path, query, fragment) {
+  if (flags & binding.URL_FLAGS_FAILED)
+    return;
+  const newIsSpecial = (flags & binding.URL_FLAGS_SPECIAL) != 0;


While you're at it, maybe != -> !== on lines 222 and 226 below?

thefourtheye · 2016-12-27T05:51:54Z

lib/internal/url.js

+  const s = this[special];
+  const ctx = this[context];
+  if ((s && !newIsSpecial) ||
+      (!s && newIsSpecial) ||


Would !!s !== !!newIsSpecial be better?

how about !s !== !newIsSpecial?

this[special] is guaranteed to be a boolean. s and !s are sufficient. There's no need (and no benefit) for getting fancy with it.

jasnell · 2016-12-27T20:02:47Z

Updated to address nits

S-YOU · 2016-12-28T02:31:44Z

lib/internal/url.js

+  if ((s && !newIsSpecial) ||
+      (!s && newIsSpecial) ||
+      (newIsSpecial && !s &&
+        ctx.host === undefined)) {


3rd one will never trigger?

ha! good catch... fixed!

targos · 2016-12-29T11:05:47Z

lib/internal/url.js

+                         host, port, path, query, fragment) {
+  if (flags & binding.URL_FLAGS_FAILED)
+    throw new TypeError('Invalid URL');
+  const ctx = this[context];


I'm surprised that we have to do this. Isn't it like caching the length of an array before a for loop (which V8 is able to optimize)?

I wouldn't say that we have to do this. This is more a code hygiene thing for me to avoid duplicated accessor calls but beyond that the impact is negligible.

jasnell · 2016-12-29T17:22:47Z

This PR will need to be updated after #10408 lands. Marking as blocked until that does.

Yields about a 25% average performance improvement

jasnell · 2016-12-30T01:04:09Z

Updated.. unblocking

jasnell · 2016-12-30T01:07:37Z

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

jasnell · 2016-12-30T02:07:07Z

Failures in freebsd are unrelated

Yields about a 25% average performance improvement PR-URL: #10469 Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell · 2016-12-30T17:50:36Z

Landed in 2213f36

mscdex · 2016-12-30T20:55:26Z

FWIW I just noticed the file mode changed again in the landed commit.

jasnell · 2016-12-30T20:57:10Z

Ugh. My git client appears to be completely ignoring my global config setting to ignore file mode changes. Getting quite frustrated with it. Thanks for the heads up

…

On Fri, Dec 30, 2016 at 12:55 PM Brian White ***@***.***> wrote: FWIW I just noticed the file mode changed again in the landed commit. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#10469 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2ecyLIEsShUdeZDZEqL7tm0v8ZGqXks5rNW_CgaJpZM4LWB-R> .

Yields about a 25% average performance improvement PR-URL: #10469 Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Dec 27, 2016

mscdex added dont-land-on-v4.x labels Dec 27, 2016

jasnell force-pushed the url-updates branch from 5e4987a to 88d9f29 Compare December 27, 2016 05:22

Trott reviewed Dec 27, 2016

View reviewed changes

thefourtheye reviewed Dec 27, 2016

View reviewed changes

jasnell force-pushed the url-updates branch from 88d9f29 to c28f1bf Compare December 27, 2016 19:58

S-YOU reviewed Dec 28, 2016

View reviewed changes

jasnell force-pushed the url-updates branch from c28f1bf to eea1ba7 Compare December 28, 2016 02:48

targos approved these changes Dec 29, 2016

View reviewed changes

targos reviewed Dec 29, 2016

View reviewed changes

jasnell added the blocked PRs that are blocked by other issues or PRs. label Dec 29, 2016

url: performance improvement in URL implementation

33b70b8

Yields about a 25% average performance improvement

jasnell force-pushed the url-updates branch from eea1ba7 to 33b70b8 Compare December 30, 2016 01:03

nodejs-github-bot added the dont-land-on-v7.x label Dec 30, 2016

jasnell removed blocked PRs that are blocked by other issues or PRs. dont-land-on-v7.x labels Dec 30, 2016

jasnell added a commit that referenced this pull request Dec 30, 2016

url: performance improvement in URL implementation

2213f36

Yields about a 25% average performance improvement PR-URL: #10469 Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell closed this Dec 30, 2016

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

evanlucas pushed a commit that referenced this pull request Jan 3, 2017

url: performance improvement in URL implementation

38d3ce9

Yields about a 25% average performance improvement PR-URL: #10469 Reviewed-By: Michaël Zasso <targos@protonmail.com>

evanlucas pushed a commit that referenced this pull request Jan 4, 2017

url: performance improvement in URL implementation

e210efa

Yields about a 25% average performance improvement PR-URL: #10469 Reviewed-By: Michaël Zasso <targos@protonmail.com>

joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: performance improvement in URL implementation #10469

url: performance improvement in URL implementation #10469

jasnell commented Dec 27, 2016

mscdex commented Dec 27, 2016

mscdex commented Dec 27, 2016 •

edited

Loading

jasnell commented Dec 27, 2016

Trott Dec 27, 2016

Trott Dec 27, 2016

thefourtheye Dec 27, 2016 •

edited

Loading

S-YOU Dec 27, 2016

jasnell Dec 27, 2016

jasnell commented Dec 27, 2016

S-YOU Dec 28, 2016

jasnell Dec 28, 2016

targos Dec 29, 2016

jasnell Dec 29, 2016

jasnell commented Dec 29, 2016

jasnell commented Dec 30, 2016

jasnell commented Dec 30, 2016

jasnell commented Dec 30, 2016

jasnell commented Dec 30, 2016

mscdex commented Dec 30, 2016

jasnell commented Dec 30, 2016 via email

url: performance improvement in URL implementation #10469

url: performance improvement in URL implementation #10469

Conversation

jasnell commented Dec 27, 2016

Checklist

Affected core subsystem(s)

Description of change

mscdex commented Dec 27, 2016

mscdex commented Dec 27, 2016 • edited Loading

jasnell commented Dec 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Dec 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Dec 29, 2016

jasnell commented Dec 30, 2016

jasnell commented Dec 30, 2016

jasnell commented Dec 30, 2016

jasnell commented Dec 30, 2016

mscdex commented Dec 30, 2016

jasnell commented Dec 30, 2016 via email

mscdex commented Dec 27, 2016 •

edited

Loading

thefourtheye Dec 27, 2016 •

edited

Loading