url: avoid instanceof for WHATWG URL #11690

mscdex · 2017-03-05T10:37:43Z

This PR replaces instanceof checks with symbol checks. Various relevant results:

                                                                             improvement confidence      p.value
 url/url-searchparams-read.js n=50000000 param="nonexistent" method="get"       195.08 %        *** 5.548226e-41
 url/url-searchparams-read.js n=50000000 param="nonexistent" method="getAll"    174.48 %        *** 3.225875e-29
 url/url-searchparams-read.js n=50000000 param="nonexistent" method="has"       190.68 %        *** 9.000669e-32
 url/url-searchparams-read.js n=50000000 param="one" method="get"               269.63 %        *** 2.180791e-33
 url/url-searchparams-read.js n=50000000 param="one" method="getAll"            111.73 %        *** 1.724534e-25
 url/url-searchparams-read.js n=50000000 param="one" method="has"               266.25 %        *** 5.498456e-33
 url/url-searchparams-read.js n=50000000 param="three" method="get"             203.56 %        *** 2.754902e-45
 url/url-searchparams-read.js n=50000000 param="three" method="getAll"          108.95 %        *** 1.790912e-35
 url/url-searchparams-read.js n=50000000 param="three" method="has"             198.65 %        *** 1.896093e-32
 url/url-searchparams-read.js n=50000000 param="two" method="get"               248.88 %        *** 4.737990e-41
 url/url-searchparams-read.js n=50000000 param="two" method="getAll"            112.93 %        *** 2.182157e-31
 url/url-searchparams-read.js n=50000000 param="two" method="has"               224.41 %        *** 2.056153e-30
 url/whatwg-url-properties.js n=10000000 prop="origin" input="auth"              12.59 %         ** 0.0035532598
 url/whatwg-url-properties.js n=10000000 prop="origin" input="dot"               10.53 %         ** 0.0041671062
 url/whatwg-url-properties.js n=10000000 prop="origin" input="file"              72.86 %        *** 0.0004049447
 url/whatwg-url-properties.js n=10000000 prop="origin" input="idn"                5.76 %            0.1494657794
 url/whatwg-url-properties.js n=10000000 prop="origin" input="javascript"        54.73 %         ** 0.0018129659
 url/whatwg-url-properties.js n=10000000 prop="origin" input="long"              10.20 %          * 0.0429791847
 url/whatwg-url-properties.js n=10000000 prop="origin" input="percent"            1.84 %            0.3920201593
 url/whatwg-url-properties.js n=10000000 prop="origin" input="short"             11.54 %        *** 0.0001064664
 url/whatwg-url-properties.js n=10000000 prop="origin" input="ws"                10.64 %         ** 0.0074980105

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

/cc @nodejs/url

Checklist

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

Affected core subsystem(s)

url-whatwg

joyeecheung

I vaguely remember some of the slowdowns of instanceof are already fixed in the v8 lkgr fork(5.8-ish)...at least for Buffer last time I played around with it. Nonethess I think testing for internal symbols is better anyway.

joyeecheung

Hit the wrong option..

TimothyGu · 2017-03-05T22:08:42Z

lib/internal/url.js

@@ -701,7 +701,7 @@ class URLSearchParams {
  }

  [util.inspect.custom](recurseTimes, ctx) {
-    if (!this || !(this instanceof URLSearchParams)) {
+    if (!this || this[searchParams] === undefined) {


The searchParams symbol is used by the URL class as well, so this will no longer error out on URL objects.

I've now added per-class symbols that are checked instead.

mscdex · 2017-03-06T02:07:00Z

CI again after symbol check changes: https://ci.nodejs.org/job/node-test-pull-request/6718/

TimothyGu · 2017-03-06T02:50:18Z

lib/internal/url.js

@@ -7,6 +7,8 @@ const context = Symbol('context');
 const cannotBeBase = Symbol('cannot-be-base');
 const special = Symbol('special');
 const searchParams = Symbol('query');
+const URLInstance = Symbol('url-instance');


I'd prefer urlInstance just to be consistent with searchParamsInstance.

TimothyGu · 2017-03-06T02:51:51Z

lib/internal/url.js

@@ -701,7 +705,7 @@ class URLSearchParams {
  }

  [util.inspect.custom](recurseTimes, ctx) {
-    if (!this || !(this instanceof URLSearchParams)) {
+    if (!this || this[searchParamsInstance] !== true) {


I'm still reluctant about writing out this[searchParamsInstance] !== true when a !this[searchParamsInstance] would do. Is this change visible performance-wise?

Just being more explicit about the expected value.

+1 to be more explicit.

Review of an older iteration of the PR

jasnell · 2017-03-06T21:59:11Z

lib/internal/url.js

@@ -7,6 +7,8 @@ const context = Symbol('context');
 const cannotBeBase = Symbol('cannot-be-base');
 const special = Symbol('special');
 const searchParams = Symbol('query');
+const urlInstance = Symbol('url-instance');
+const searchParamsInstance = Symbol('searchParams-instance');


Why not use the existing context and searchParams symbols rather than introducing new ones? All URL objects should have the context symbol set. Likewise all URLSearchParams instances should have the searchParams symbol set (I believe)

As @TimothyGu noted, both classes use both of those symbols, so they don't currently each have a unique symbol?

You can probably separate them like this:

if (url[searchParams]) { if (url[searchParams][searchParams]) { } // URL, it's an array else {} // URLSearchParams, it's undefined }

TimothyGu

Any updates on this?

TimothyGu · 2017-03-15T01:46:16Z

lib/internal/url.js

@@ -219,7 +221,8 @@ class URL {
  constructor(input, base) {
    // toUSVString is not needed.
    input = '' + input;
-    if (base !== undefined && !(base instanceof URL))
+    this[urlInstance] = true;


I might put the urlInstance in the prototype to avoid having to set this every single time an object is created.

Then URL.prototype will pass the brand check, which seems bad.

TimothyGu · 2017-03-15T01:46:44Z

lib/internal/url.js

@@ -646,6 +649,7 @@ class URLSearchParams {
  // Default parameter is necessary to keep URLSearchParams.length === 0 in
  // accordance with Web IDL spec.
  constructor(init = undefined) {
+    this[searchParamsInstance] = true;


mscdex · 2017-03-29T09:24:22Z

Alright, I've switched to checking using only searchParams. New results with V8 5.7:

                                                                              improvement confidence      p.value
 url/url-searchparams-read.js n=20000000 param="nonexistent" method="get"       163.51 %        *** 2.269241e-61
 url/url-searchparams-read.js n=20000000 param="nonexistent" method="getAll"    160.05 %        *** 5.142371e-51
 url/url-searchparams-read.js n=20000000 param="nonexistent" method="has"       167.00 %        *** 1.355736e-46
 url/url-searchparams-read.js n=20000000 param="one" method="get"               236.25 %        *** 8.197452e-52
 url/url-searchparams-read.js n=20000000 param="one" method="getAll"            120.78 %        *** 2.622135e-51
 url/url-searchparams-read.js n=20000000 param="one" method="has"               243.64 %        *** 6.676611e-50
 url/url-searchparams-read.js n=20000000 param="three" method="get"             194.93 %        *** 5.614993e-59
 url/url-searchparams-read.js n=20000000 param="three" method="getAll"          106.35 %        *** 3.023894e-81
 url/url-searchparams-read.js n=20000000 param="three" method="has"             189.66 %        *** 7.362414e-46
 url/url-searchparams-read.js n=20000000 param="two" method="get"               218.28 %        *** 1.350097e-67
 url/url-searchparams-read.js n=20000000 param="two" method="getAll"            113.33 %        *** 2.637819e-83
 url/url-searchparams-read.js n=20000000 param="two" method="has"               218.26 %        *** 4.443562e-86
 url/whatwg-url-properties.js n=300000 prop="origin" input="auth"                12.40 %        *** 3.767663e-16
 url/whatwg-url-properties.js n=300000 prop="origin" input="dot"                  7.74 %        *** 8.915982e-09
 url/whatwg-url-properties.js n=300000 prop="origin" input="file"                77.36 %        *** 1.300946e-26
 url/whatwg-url-properties.js n=300000 prop="origin" input="idn"                  4.96 %        *** 2.077301e-10
 url/whatwg-url-properties.js n=300000 prop="origin" input="javascript"          83.97 %        *** 6.470616e-48
 url/whatwg-url-properties.js n=300000 prop="origin" input="long"                 9.44 %        *** 3.128818e-11
 url/whatwg-url-properties.js n=300000 prop="origin" input="percent"              7.26 %        *** 3.400718e-14
 url/whatwg-url-properties.js n=300000 prop="origin" input="short"                9.22 %        *** 1.478553e-11
 url/whatwg-url-properties.js n=300000 prop="origin" input="ws"                   7.70 %        *** 9.628009e-09

Results are pretty close to what they were before.

jasnell

Generally LGTM with a question/suggestion

jasnell · 2017-03-29T20:41:08Z

lib/internal/url.js

@@ -239,8 +239,10 @@ class URL {
  constructor(input, base) {
    // toUSVString is not needed.
    input = `${input}`;
-    if (base !== undefined && !(base instanceof URL))
+    if (base !== undefined &&
+        (!base[searchParams] || !base[searchParams][searchParams])) {


@mscdex ... is there benefit to moving this into a separate isURL(u) function that can be exported by internal/url?

Performance-wise? No. There's actually a small but measurable performance hit when extracting all of the logic to a separate function, compared to having it inline. For example:

improvement confidence p.value url/url-searchparams-read.js n=20000000 param="nonexistent" method="get" 165.07 % *** 1.189398e-66 url/url-searchparams-read.js n=20000000 param="nonexistent" method="getAll" 139.46 % *** 4.331942e-46 url/url-searchparams-read.js n=20000000 param="nonexistent" method="has" 161.48 % *** 2.741360e-52 url/url-searchparams-read.js n=20000000 param="one" method="get" 222.70 % *** 3.111318e-46 url/url-searchparams-read.js n=20000000 param="one" method="getAll" 113.31 % *** 1.122968e-53 url/url-searchparams-read.js n=20000000 param="one" method="has" 232.04 % *** 1.571222e-55 url/url-searchparams-read.js n=20000000 param="three" method="get" 184.13 % *** 2.078669e-48 url/url-searchparams-read.js n=20000000 param="three" method="getAll" 100.34 % *** 4.591714e-56 url/url-searchparams-read.js n=20000000 param="three" method="has" 181.29 % *** 7.340053e-43 url/url-searchparams-read.js n=20000000 param="two" method="get" 209.77 % *** 4.581349e-54 url/url-searchparams-read.js n=20000000 param="two" method="getAll" 102.89 % *** 2.001755e-36 url/url-searchparams-read.js n=20000000 param="two" method="has" 203.09 % *** 1.810527e-44

joyeecheung

Still LGTM

joyeecheung · 2017-03-30T10:08:10Z

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

jasnell · 2017-04-04T18:06:38Z

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

TimothyGu · 2017-04-05T00:37:06Z

Landed in b76a350.

PR-URL: #11690 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

italoacasas · 2017-04-10T22:08:58Z

cc @mscdex

guybedford · 2018-08-28T18:36:22Z

This has come up again in #22506. @mscdex do you think we could move to an instanceof check at this point from a performance perspective yet? Would value your feedback on this.

mscdex · 2018-08-28T21:56:10Z

@guybedford I do not know offhand. Someone would have to be sure to run benchmarks on both implementations and compare the results for master.

devsnek · 2018-08-28T22:04:33Z

I think there's also a concern about Object.create(URL.prototype) matching instanceof. If we use instanceof and then try to access our symbol properties it will fail.

mscdex added performance Issues and PRs related to the performance of Node.js. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 5, 2017

nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 5, 2017

joyeecheung reviewed Mar 5, 2017

View reviewed changes

joyeecheung approved these changes Mar 5, 2017

View reviewed changes

addaleax mentioned this pull request Mar 5, 2017

stream: support Uint8Array input to methods #11608

Closed

4 tasks

TimothyGu previously requested changes Mar 5, 2017

View reviewed changes

mscdex force-pushed the url-whatwg-perf branch from 9220505 to c115e49 Compare March 6, 2017 02:04

TimothyGu reviewed Mar 6, 2017

View reviewed changes

mscdex force-pushed the url-whatwg-perf branch from c115e49 to 1f9ba08 Compare March 6, 2017 02:59

TimothyGu approved these changes Mar 6, 2017

View reviewed changes

jasnell requested changes Mar 6, 2017

View reviewed changes

TimothyGu reviewed Mar 15, 2017

View reviewed changes

url: avoid instanceof for WHATWG URL

3d067a6

mscdex force-pushed the url-whatwg-perf branch from 1f9ba08 to 3d067a6 Compare March 29, 2017 09:21

jasnell approved these changes Mar 29, 2017

View reviewed changes

joyeecheung approved these changes Mar 30, 2017

View reviewed changes

watilde approved these changes Apr 5, 2017

View reviewed changes

TimothyGu closed this Apr 5, 2017

mscdex deleted the url-whatwg-perf branch April 5, 2017 04:09

italoacasas added the backport-to-v7.x label Apr 10, 2017

TimothyGu mentioned this pull request Apr 19, 2017

(v7.x backport) url: updates to the WHATWG URL parser #12507

Merged

4 tasks

TimothyGu added backported-to-v7.x and removed backport-requested-v7.x labels Apr 27, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

guybedford mentioned this pull request Aug 26, 2018

url: expose pathToFileURL and fileURLToPath #22506

Closed

4 tasks

aliscco mentioned this pull request Jun 23, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#571

Open

leonardoadame mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#377

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#1015

Open

aliscco mentioned this pull request Jan 9, 2024

[Snyk] Security upgrade webpack from 4.47.0 to 5.1.1 aliscco/alisco-node#1197

Open

leonardoadame mentioned this pull request Jan 10, 2024

[Snyk] Security upgrade webpack from 4.47.0 to 5.1.1 leonardoadame/node#621

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: avoid instanceof for WHATWG URL #11690

url: avoid instanceof for WHATWG URL #11690

mscdex commented Mar 5, 2017 •

edited

Loading

joyeecheung left a comment

joyeecheung left a comment

TimothyGu Mar 5, 2017

mscdex Mar 6, 2017

mscdex commented Mar 6, 2017

TimothyGu Mar 6, 2017

mscdex Mar 6, 2017

TimothyGu Mar 6, 2017

mscdex Mar 6, 2017

joyeecheung Mar 6, 2017

jasnell Mar 6, 2017

mscdex Mar 7, 2017 •

edited

Loading

joyeecheung Mar 7, 2017

TimothyGu left a comment

TimothyGu Mar 15, 2017

domenic Mar 15, 2017

TimothyGu Mar 15, 2017

mscdex commented Mar 29, 2017

jasnell left a comment

jasnell Mar 29, 2017

mscdex Mar 30, 2017

joyeecheung left a comment

joyeecheung commented Mar 30, 2017

jasnell commented Apr 4, 2017

TimothyGu commented Apr 5, 2017

italoacasas commented Apr 10, 2017

guybedford commented Aug 28, 2018

mscdex commented Aug 28, 2018

devsnek commented Aug 28, 2018

url: avoid instanceof for WHATWG URL #11690

url: avoid instanceof for WHATWG URL #11690

Conversation

mscdex commented Mar 5, 2017 • edited Loading

Checklist

Affected core subsystem(s)

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Mar 6, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

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

mscdex commented Mar 29, 2017

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung commented Mar 30, 2017

jasnell commented Apr 4, 2017

TimothyGu commented Apr 5, 2017

italoacasas commented Apr 10, 2017

guybedford commented Aug 28, 2018

mscdex commented Aug 28, 2018

devsnek commented Aug 28, 2018

mscdex commented Mar 5, 2017 •

edited

Loading

mscdex Mar 7, 2017 •

edited

Loading