Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

url: don't update URL immediately on update to URLSearchParams #51520

Merged
merged 18 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2b2d11b
url: remove #context from URLSearchParams
MattIPv4 Jan 19, 2024
129a1a9
url: rely on URLSearchParams if in use for search
MattIPv4 Jan 19, 2024
c2e8526
url: update URLSearchParams directly from URL search setter
MattIPv4 Jan 19, 2024
039380e
url: create href using searchParams without bindingUrl call
MattIPv4 Jan 19, 2024
e015de2
url: ensure ? is present when relying on URLSearchParams
MattIPv4 Jan 19, 2024
2487726
url: don't use public getter for toString/toJSON
MattIPv4 Jan 19, 2024
b0270bc
url: update searchParams tests to expect encoded results
MattIPv4 Jan 19, 2024
d9c5381
url: only use URLSearchParams as store when modified
MattIPv4 Jan 19, 2024
a05675c
url: store search in URL on first access/update after URLSearchParams…
MattIPv4 Jan 19, 2024
310d275
url: add benchmark for param append for URL and URLSearchParams
MattIPv4 Jan 19, 2024
b118a90
url: add benchmark to check impact of searchParams checking
MattIPv4 Jan 19, 2024
916374e
url: reduce diff for changes
MattIPv4 Jan 19, 2024
319eaa0
url: more comments to explain lazy searchParams updating
MattIPv4 Jan 19, 2024
0c44fb6
url: cleanup from review
MattIPv4 Jan 19, 2024
065c45c
url: update variable naming, add more comments, include hash in bench…
MattIPv4 Jan 19, 2024
0d0fef8
url: add tests to ensure lazy URLSearchParams is exercised
MattIPv4 Jan 19, 2024
e9c4da2
url: fix updating url hash etc. after searchParams update, add test
MattIPv4 Jan 21, 2024
04f53a7
url: update tests for invalid this
MattIPv4 Jan 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions benchmark/url/url-searchparams-append.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';
const common = require('../common.js');

const bench = common.createBenchmark(main, {
type: ['URL', 'URLSearchParams'],
n: [1e3, 1e6],
});

function main({ type, n }) {
const params = type === 'URL' ?
new URL('https://nodejs.org').searchParams :
new URLSearchParams();

bench.start();
for (let i = 0; i < n; i++) {
params.append('test', i);
}
bench.end(n);
}
23 changes: 23 additions & 0 deletions benchmark/url/url-searchparams-update.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';
const common = require('../common.js');

const bench = common.createBenchmark(main, {
searchParams: ['true', 'false'],
property: ['pathname', 'search'],
MattIPv4 marked this conversation as resolved.
Show resolved Hide resolved
n: [1e6],
});

function main({ searchParams, property, n }) {
const url = new URL('https://nodejs.org');
if (searchParams === 'true') url.searchParams; // eslint-disable-line no-unused-expressions
MattIPv4 marked this conversation as resolved.
Show resolved Hide resolved

const method = property === 'pathname' ?
(x) => url.pathname = `/${x}` :
(x) => url.search = `?${x}`;

bench.start();
for (let i = 0; i < n; i++) {
method(i);
}
bench.end(n);
}
78 changes: 63 additions & 15 deletions lib/internal/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ class URLContext {
}
}

let setURLSearchParamsModified;
let setURLSearchParamsContext;
let getURLSearchParamsList;
let setURLSearchParams;
Expand Down Expand Up @@ -475,8 +476,9 @@ class URLSearchParams {
name = StringPrototypeToWellFormed(`${name}`);
value = StringPrototypeToWellFormed(`${value}`);
ArrayPrototypePush(this.#searchParams, name, value);

if (this.#context) {
this.#context.search = this.toString();
setURLSearchParamsModified(this.#context);
}
}

Expand Down Expand Up @@ -509,8 +511,9 @@ class URLSearchParams {
}
}
}

if (this.#context) {
this.#context.search = this.toString();
setURLSearchParamsModified(this.#context);
}
}

Expand Down Expand Up @@ -615,7 +618,7 @@ class URLSearchParams {
}

if (this.#context) {
this.#context.search = this.toString();
setURLSearchParamsModified(this.#context);
}
}

Expand Down Expand Up @@ -664,7 +667,7 @@ class URLSearchParams {
}

if (this.#context) {
this.#context.search = this.toString();
setURLSearchParamsModified(this.#context);
}
}

Expand Down Expand Up @@ -769,6 +772,20 @@ function isURL(self) {
class URL {
#context = new URLContext();
#searchParams;
#searchParamsModified;

static {
setURLSearchParamsModified = (obj) => {
// When URLSearchParams changes, we lazily update URL on the next read/write for performance.
obj.#searchParamsModified = true;

// If URL has an existing search, remove it without cascading back to URLSearchParams.
// Do this to avoid any internal confusion about whether URLSearchParams or URL is up-to-date.
if (obj.#context.hasSearch) {
obj.#updateContext(bindingUrl.update(obj.#context.href, updateActions.kSearch, ''), false);
}
};
}

constructor(input, base = undefined) {
markTransferMode(this, false, false);
Expand Down Expand Up @@ -814,7 +831,33 @@ class URL {
return `${constructor.name} ${inspect(obj, opts)}`;
}

#updateContext(href) {
#getSearchFromContext() {
if (!this.#context.hasSearch) return '';
let endsAt = this.#context.href.length;
if (this.#context.hasHash) endsAt = this.#context.hash_start;
if (endsAt - this.#context.search_start <= 1) return '';
return StringPrototypeSlice(this.#context.href, this.#context.search_start, endsAt);
}

#getSearchFromParams() {
if (!this.#searchParams?.size) return '';
return `?${this.#searchParams}`;
}

#checkSearchParams() {
MattIPv4 marked this conversation as resolved.
Show resolved Hide resolved
// If URLSearchParams has been modified, reflect that back into URL, and do not cascade back.
// This is done lazily to greatly improve performance when URLSearchParams is updated repeatedly.
if (this.#searchParamsModified) {
const href = bindingUrl.update(this.#context.href, updateActions.kSearch, this.#getSearchFromParams());
this.#updateContext(href, false);
this.#searchParamsModified = false;
}
}

#updateContext(href, updateSearchParams = !!this.#searchParams) {
MattIPv4 marked this conversation as resolved.
Show resolved Hide resolved
const previousSearch = updateSearchParams && (this.#searchParamsModified ?
this.#getSearchFromParams() : this.#getSearchFromContext());

this.#context.href = href;

const {
Expand All @@ -839,20 +882,26 @@ class URL {
this.#context.hash_start = hash_start;
this.#context.scheme_type = scheme_type;

if (this.#searchParams) {
if (this.#context.hasSearch) {
setURLSearchParams(this.#searchParams, this.search);
// If URLSearchParams exists, and the search has changed, update it.
// Otherwise, check if URLSearchParams has been modified.
if (updateSearchParams) {
MattIPv4 marked this conversation as resolved.
Show resolved Hide resolved
const currentSearch = this.#getSearchFromContext();
if (previousSearch !== currentSearch) {
setURLSearchParams(this.#searchParams, currentSearch);
this.#searchParamsModified = false;
} else {
setURLSearchParams(this.#searchParams, undefined);
this.#checkSearchParams();
}
}
}

toString() {
this.#checkSearchParams();
return this.#context.href;
}

get href() {
this.#checkSearchParams();
return this.#context.href;
}

Expand Down Expand Up @@ -1002,11 +1051,8 @@ class URL {
}

get search() {
if (!this.#context.hasSearch) { return ''; }
let endsAt = this.#context.href.length;
if (this.#context.hasHash) { endsAt = this.#context.hash_start; }
if (endsAt - this.#context.search_start <= 1) { return ''; }
return StringPrototypeSlice(this.#context.href, this.#context.search_start, endsAt);
this.#checkSearchParams();
return this.#getSearchFromContext();
}

set search(value) {
Expand All @@ -1020,8 +1066,9 @@ class URL {
get searchParams() {
// Create URLSearchParams on demand to greatly improve the URL performance.
if (this.#searchParams == null) {
this.#searchParams = new URLSearchParams(this.search);
this.#searchParams = new URLSearchParams(this.#getSearchFromContext());
setURLSearchParamsContext(this.#searchParams, this);
this.#searchParamsModified = false;
}
return this.#searchParams;
}
Expand All @@ -1041,6 +1088,7 @@ class URL {
}

toJSON() {
this.#checkSearchParams();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function can just be this.href.

Copy link
Member Author

@MattIPv4 MattIPv4 Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I fine to also swap toString to using href, even though its technically defined after?

or should href and toJSON both switch to being backed by toString?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toJSON and toString should call this.href, since technically they are href

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching these over to return this.href results in some WPT failures 🤔

[UNEXPECTED_FAILURE][FAIL] URL interface: stringifier
assert_throws_js: calling stringifier with this = {} didn't throw TypeError function "function() {
            interfacePrototypeObject.toString.apply({}, []);
        }" did not throw
    at IdlInterface.<anonymous> (/Users/mattcowley/GitHub/nodejs/node/test/fixtures/wpt/resources/idlharness.js:2664:9)
    at Test.step (/Users/mattcowley/GitHub/nodejs/node/test/fixtures/wpt/resources/testharness.js:2599:25)
    at test (/Users/mattcowley/GitHub/nodejs/node/test/fixtures/wpt/resources/testharness.js:628:30)
    at self.subsetTestByKey (/Users/mattcowley/GitHub/nodejs/node/test/fixtures/wpt/resources/idlharness.js:44:14)
Command: /Users/mattcowley/GitHub/nodejs/node/out/Release/node  /Users/mattcowley/GitHub/nodejs/node/test/wpt/test-url.js 'idlharness.any.js'

[UNEXPECTED_FAILURE][FAIL] URL interface: operation toJSON()
assert_throws_js: calling operation with this = {} didn't throw TypeError function "function() {
            fn.apply(obj, args);
        }" did not throw
    at throwOrReject (/Users/mattcowley/GitHub/nodejs/node/test/fixtures/wpt/resources/idlharness.js:107:9)
    at IdlInterface.do_member_operation_asserts (/Users/mattcowley/GitHub/nodejs/node/test/fixtures/wpt/resources/idlharness.js:2405:9)
    at IdlInterface.<anonymous> (/Users/mattcowley/GitHub/nodejs/node/test/fixtures/wpt/resources/idlharness.js:2309:14)
    at Test.step (/Users/mattcowley/GitHub/nodejs/node/test/fixtures/wpt/resources/testharness.js:2599:25)
Command: /Users/mattcowley/GitHub/nodejs/node/out/Release/node  /Users/mattcowley/GitHub/nodejs/node/test/wpt/test-url.js 'idlharness.any.js'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you need private property access to check if the current this is same as URL. I forgot about this!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I do this though, the WPT tests pass, but parallel/test-whatwg-url-invalidthis is still failing:

  get #href() {
    // Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
    this.#ensureSearchParamsUpdated();
    return this.#context.href;
  }

  toString() {
    return this.#href;
  }

  get href() {
    return this.#href;
  }

...

  toJSON() {
    return this.#href;
  }
~/GitHub/nodejs/node url-searchparams-perf !2 ❯ tools/test.py parallel/test-whatwg-url-invalidthis                                                                                         3s
=== release test-whatwg-url-invalidthis ===                   
Path: parallel/test-whatwg-url-invalidthis
node:assert:644
      throw err;
      ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  Comparison {
+   message: 'Receiver must be an instance of class URL',
-   message: /Cannot read private member/,
    name: 'TypeError'
  }
    at /Users/mattcowley/GitHub/nodejs/node/test/parallel/test-whatwg-url-invalidthis.js:12:10
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/Users/mattcowley/GitHub/nodejs/node/test/parallel/test-whatwg-url-invalidthis.js:11:3)
    at Module._compile (node:internal/modules/cjs/loader:1378:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1437:10)
    at Module.load (node:internal/modules/cjs/loader:1212:32)
    at Module._load (node:internal/modules/cjs/loader:1028:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: TypeError: Receiver must be an instance of class URL
      at Array.toString (node:internal/url:915:17)
      at assert.throws.name (/Users/mattcowley/GitHub/nodejs/node/test/parallel/test-whatwg-url-invalidthis.js:12:31)
      at getActual (node:assert:765:5)
      at Function.throws (node:assert:911:24)
      at /Users/mattcowley/GitHub/nodejs/node/test/parallel/test-whatwg-url-invalidthis.js:12:10
      at Array.forEach (<anonymous>)
      at Object.<anonymous> (/Users/mattcowley/GitHub/nodejs/node/test/parallel/test-whatwg-url-invalidthis.js:11:3)
      at Module._compile (node:internal/modules/cjs/loader:1378:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1437:10)
      at Module.load (node:internal/modules/cjs/loader:1212:32),
  expected: { name: 'TypeError', message: /Cannot read private member/ },
  operator: 'throws'
}

Node.js v22.0.0-pre
Command: out/Release/node /Users/mattcowley/GitHub/nodejs/node/test/parallel/test-whatwg-url-invalidthis.js


[00:00|% 100|+   0|-   1]: Done                               

Failed tests:
out/Release/node /Users/mattcowley/GitHub/nodejs/node/test/parallel/test-whatwg-url-invalidthis.js

Copy link
Member Author

@MattIPv4 MattIPv4 Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking this through more, is this throwing a different exception now because it is trying to access a private method first (#ensureSearchParamsUpdated), instead of a private property (#context)? If so, is it fine to just update the test to expect this new exception?

return this.#context.href;
}

Expand Down
Loading