url: refactor to use more primordials #45966

aduh95 · 2022-12-24T20:48:02Z

No description provided.

anonrig

This pull request includes changes more than just refactoring primordial, and might confuse reviewers. I recommend either changing the title of this pull request or introducing changes other than primordials to a different pull request.

Can you also run a benchmark?

anonrig · 2022-12-24T23:35:23Z

lib/internal/url.js

+  if (!isWindows) filepath = StringPrototypeReplaceAll(filepath, '\\', '%5C');
+  filepath = StringPrototypeReplaceAll(filepath, '\n', '%0A');
+  filepath = StringPrototypeReplaceAll(filepath, '\r', '%0D');
+  filepath = StringPrototypeReplaceAll(filepath, '\t', '%09');


According to micro-benchmarks, ReplaceAll is slower than Replace. I prefer using the existing implementation. Referencing: https://github.com/RafaelGSS/nodejs-bench-operations/blob/main/RESULTS-v19.md

The existing implementation relies on user-mutable RegExp.prototype[Symbol.replace]. According to my own research, replaceAll on a string was faster than building a robust regex. (Actually in this case we're reusing the same regex, so it should not really matter, let's go with the regex approach)

lib/internal/url.js

anonrig · 2022-12-24T23:37:09Z

lib/internal/url.js

    outURL.hostname = domainToASCII(hostname);
    outURL.pathname = encodePathChars(
-      ArrayPrototypeJoin(ArrayPrototypeSlice(paths, 3), '/'));
+      StringPrototypeReplaceAll(StringPrototypeSlice(filepath, hostnameEndIndex), '\\', '/'));


This pull request changes more than just refactoring primordials.

StringPrototypeSplit relies on user-mutable String.prototype[Symbol.split], so it's not exactly unrelated.

aduh95 · 2022-12-25T00:59:09Z

Can you also run a benchmark?

FYI that's why I added needs-benchmark-ci PR that need a benchmark CI run. .

…eAll`

nodejs-github-bot · 2022-12-26T10:08:59Z

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

joyeecheung · 2022-12-26T16:57:31Z

Do we actually have benchmarks for url.fileURLToPath() and url.pathToFileURL()? I can't find anything after searching in benchmark/, but they are used a lot internally and are what this PR primarily affects now.

joyeecheung

We need to benchmark url.fileURLToPath() and url.pathToFileURL(), they are used a lot internally and are affected by this PR

lib/internal/url.js

nodejs-github-bot · 2023-01-19T15:37:57Z

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

This reverts commit 1ceaa2c and 1fc0fb0.

nodejs-github-bot · 2023-01-22T10:44:13Z

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

nodejs-github-bot · 2023-01-22T12:09:57Z

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

aduh95 · 2023-01-22T13:13:36Z

Landed in e487638

PR-URL: #45966 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

url: refactor to use more primordials

44655cc

aduh95 added whatwg-url Issues and PRs related to the WHATWG URL implementation. needs-benchmark-ci PR that need a benchmark CI run. labels Dec 24, 2022

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 24, 2022

himself65 approved these changes Dec 24, 2022

View reviewed changes

anonrig requested changes Dec 24, 2022

View reviewed changes

aduh95 added 2 commits December 25, 2022 02:16

Use RegExpPrototypeSymbolReplace over StringPrototypeReplaceAll

80452de

fixup! Use RegExpPrototypeSymbolReplace over `StringPrototypeReplac…

2475de3

…eAll`

anonrig approved these changes Dec 25, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

do not harden in case that helps

b2ab16c

aduh95 force-pushed the url-primordials branch from 0008c09 to b2ab16c Compare December 26, 2022 00:37

This comment was marked as outdated.

Sign in to view

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 26, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2022

joyeecheung requested changes Dec 26, 2022

View reviewed changes

This was referenced Dec 27, 2022

CI Reliability 2022-12-27 nodejs/reliability#470

Open

CI Reliability 2022-12-28 nodejs/reliability#471

Open

CI Reliability 2022-12-29 nodejs/reliability#472

Open

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Dec 30, 2022

CI Reliability 2022-12-30 nodejs/reliability#473

Open

10 tasks

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Dec 31, 2022

CI Reliability 2022-12-31 nodejs/reliability#474

Open

20 tasks

add benchmarks for url utils

b8d193f

aduh95 force-pushed the url-primordials branch from 1de5314 to b8d193f Compare December 31, 2022 06:50

github-actions bot mentioned this pull request Jan 19, 2023

CI Reliability 2023-01-19 nodejs/reliability#488

Open

14 tasks

aduh95 commented Jan 19, 2023

View reviewed changes

lib/internal/url.js Outdated Show resolved Hide resolved

aduh95 added 2 commits January 19, 2023 15:23

Update lib/internal/url.js

00cd18f

esm: skip file: URL conversion to path when possible

1fc0fb0

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023

This was referenced Jan 20, 2023

CI Reliability 2023-01-20 nodejs/reliability#489

Open

CI Reliability 2023-01-21 nodejs/reliability#490

Open

CI Reliability 2023-01-22 nodejs/reliability#491

Open

aduh95 added 4 commits January 22, 2023 10:26

fix UNC validation

3a61d80

fixup! esm: skip file: URL conversion to path when possible

1ceaa2c

side effect free regex replacement

fc4fc3f

Revert "esm: skip file: URL conversion to path when possible"

256e4bd

This reverts commit 1ceaa2c and 1fc0fb0.

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 22, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2023

aduh95 merged commit e487638 into nodejs:main Jan 22, 2023

aduh95 deleted the url-primordials branch January 22, 2023 13:13

This was referenced Jan 23, 2023

CI Reliability 2023-01-23 nodejs/reliability#492

Open

CI Reliability 2023-01-24 nodejs/reliability#493

Open

CI Reliability 2023-01-25 nodejs/reliability#494

Open

ruyadorno pushed a commit that referenced this pull request Feb 1, 2023

url: refactor to use more primordials

e460d16

PR-URL: #45966 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

ruyadorno mentioned this pull request Feb 1, 2023

v19.6.0 proposal #46455

Merged

juanarbol pushed a commit that referenced this pull request Mar 3, 2023

url: refactor to use more primordials

c4a39fa

PR-URL: #45966 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

juanarbol mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

juanarbol pushed a commit that referenced this pull request Mar 5, 2023

url: refactor to use more primordials

08bafc8

PR-URL: #45966 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: refactor to use more primordials #45966

url: refactor to use more primordials #45966

aduh95 commented Dec 24, 2022

anonrig left a comment

anonrig Dec 24, 2022

aduh95 Dec 25, 2022 •

edited

anonrig Dec 24, 2022 •

edited

aduh95 Dec 25, 2022

aduh95 commented Dec 25, 2022

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Dec 26, 2022

joyeecheung commented Dec 26, 2022 •

edited

joyeecheung left a comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Jan 19, 2023

nodejs-github-bot commented Jan 22, 2023

nodejs-github-bot commented Jan 22, 2023

aduh95 commented Jan 22, 2023

url: refactor to use more primordials #45966

url: refactor to use more primordials #45966

Conversation

aduh95 commented Dec 24, 2022

anonrig left a comment

Choose a reason for hiding this comment

anonrig Dec 24, 2022

Choose a reason for hiding this comment

aduh95 Dec 25, 2022 • edited

Choose a reason for hiding this comment

anonrig Dec 24, 2022 • edited

Choose a reason for hiding this comment

aduh95 Dec 25, 2022

Choose a reason for hiding this comment

aduh95 commented Dec 25, 2022

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Dec 26, 2022

joyeecheung commented Dec 26, 2022 • edited

joyeecheung left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Jan 19, 2023

nodejs-github-bot commented Jan 22, 2023

nodejs-github-bot commented Jan 22, 2023

aduh95 commented Jan 22, 2023

aduh95 Dec 25, 2022 •

edited

anonrig Dec 24, 2022 •

edited

joyeecheung commented Dec 26, 2022 •

edited