esm: avoid try/catch when validating urls #47541

anonrig · 2023-04-13T14:39:13Z

Due to URL.canParse, we can avoid try/catch block and have faster validation. The previous implementation was not performant due to:

Unnecessary string serialization - We don't need href, origin etc. for validating if a URL is valid or not.
URL.canParse can be written with V8 Fast API - enabling more performance out of this pull request.
URL.canParse does not return anything except a boolean.

cc @nodejs/url

nodejs-github-bot · 2023-04-13T14:39:18Z

Review requested:

@nodejs/loaders
@nodejs/modules

aduh95

Given that URL.canParse is user-mutable, should we use const { canParse } = URL or something like that? Or even better, can we import it primordial-style from ada directly?

anonrig · 2023-04-13T14:47:59Z

Given that URL.canParse is user-mutable, should we use const { canParse } = URL or something like that? Or even better, can we import it primordial-style from ada directly?

@aduh95 I like where this is going, but I couldn't visualize it. Can you elaborate on how we can import it primordial-style? We can always do const { canParse } = internalBinding('url'); if that's what you are recommending?

aduh95 · 2023-04-13T15:30:53Z

Yes, I mean something that's available only internally and cannot be mutated at runtime. const { canParse } = internalBinding('url') is close enough, let's roll with this :)

nodejs-github-bot · 2023-04-13T16:04:01Z

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

ljharb · 2023-04-13T17:27:30Z

This is way better/cleaner :-) but does it prevent backporting this commit, since older release lines may not have URL.canParse?

anonrig · 2023-04-13T17:44:32Z

This is way better/cleaner :-) but does it prevent backporting this commit, since older release lines may not have URL.canParse?

It might make it harder, due to conflicts, but I don't think it will block backporting any future changes to ESM.

ljharb · 2023-04-13T17:46:21Z

oh sure, i just meant, should this PR have any "do not backport" labels, or link to the canParse PR as a prereq?

anonrig · 2023-04-13T18:02:07Z

oh sure, i just meant, should this PR have any "do not backport" labels, or link to the canParse PR as a prereq?

I'm not quite sure. @nodejs/releasers what do you recommend?

JakobJingleheimer

Sweet, thanks! I was just suggesting this change last night in Slack 😁

aduh95 · 2023-04-13T20:27:58Z

oh sure, i just meant, should this PR have any "do not backport" labels, or link to the canParse PR as a prereq?

canParse was added in #47179, and there's no labels that says the backport is blocked, so no action should be necessary here I think. PRs are backported in the order they landed on Current release lines, precisely because it's often the case that a PR builds on top of another one.

mscdex · 2023-04-13T20:50:57Z

2. URL.canParse can be written with V8 Fast API - enabling more performance out of this pull request.

I'm confused by this. The C++ API still says this:

node/src/node_url.cc

Lines 116 to 117 in 4afb25c

    
           // TODO(@anonrig): Add V8 Fast API for CanParse method 
        
           void BindingData::CanParse(const FunctionCallbackInfo<Value>& args) {

Or did you actually mean this PR does not improve performance until the TODO is resolved?

anonrig · 2023-04-13T21:09:32Z

Or did you actually mean this PR does not improve performance until the TODO is resolved?

I was giving some context to this particular change. It is definitely faster than the current implementation. Additionally, @KhafraDev just opened a pull request for adding v8 fast API to canParse #47552

ljharb · 2023-04-13T21:20:50Z

@aduh95 perfect, thanks for clarifying and linking :-)

nodejs-github-bot · 2023-04-14T15:04:52Z

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

nodejs-github-bot · 2023-04-14T20:26:06Z

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

anonrig · 2023-04-14T22:25:25Z

Would someone re-review this pull request? It's required due to force-push.

JakobJingleheimer · 2023-04-15T10:49:20Z

lib/internal/modules/esm/hooks.js

@@ -35,6 +35,7 @@ const {
 } = require('internal/errors').codes;
 const { exitCodes: { kUnfinishedTopLevelAwait } } = internalBinding('errors');
 const { URL } = require('internal/url');
+const { canParse: urlCanParse } = internalBinding('url');


nit: this sounds a little Yoda-esque

Suggested change

const { canParse: urlCanParse } = internalBinding('url');

const { canParse: canParseURL } = internalBinding('url');

Not sure this is better, isURLString might be more suited. URLCanParse has the upside of being consistent with how primordials are named, which is nice imho

I want to change this to URLCanParse too but I'm too demotivated by having to run Node.js CI multiple times because of the flaky tests.

nodejs-github-bot · 2023-04-15T14:43:21Z

Landed in 17570c0

PR-URL: #47541 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

PR-URL: nodejs#47541 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

PR-URL: #47541 Backport-PR-URL: #50669 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

PR-URL: nodejs/node#47541 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Apr 13, 2023

aduh95 reviewed Apr 13, 2023

View reviewed changes

anonrig force-pushed the improve-esm-performance branch from 5375959 to 3a57762 Compare April 13, 2023 15:52

anonrig requested a review from aduh95 April 13, 2023 15:53

aduh95 approved these changes Apr 13, 2023

View reviewed changes

anonrig 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 Apr 13, 2023

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

anonrig mentioned this pull request Apr 13, 2023

url: handle URL.canParse without base parameter #47547

Merged

JakobJingleheimer approved these changes Apr 13, 2023

View reviewed changes

lpinca approved these changes Apr 13, 2023

View reviewed changes

esm: avoid try/catch when validating urls

05742e9

anonrig force-pushed the improve-esm-performance branch from 3a57762 to 05742e9 Compare April 14, 2023 13:45

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2023

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

anonrig requested review from lpinca, aduh95 and JakobJingleheimer April 14, 2023 22:24

Qard approved these changes Apr 14, 2023

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 14, 2023

github-actions bot mentioned this pull request Apr 15, 2023

CI Reliability 2023-04-15 nodejs/reliability#543

Open

33 tasks

JakobJingleheimer approved these changes Apr 15, 2023

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2023

nodejs-github-bot merged commit 17570c0 into nodejs:main Apr 15, 2023
27 checks passed

github-actions bot mentioned this pull request Apr 18, 2023

CI Reliability 2023-04-18 nodejs/reliability#544

Open

35 tasks

targos mentioned this pull request May 2, 2023

v20.1.0 release proposal #47820

Merged

danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 3, 2023

danielleadams mentioned this pull request Jul 3, 2023

esm: rename URLCanParse to be consistent #47668

Merged

targos mentioned this pull request Nov 11, 2023

[v18.x] Backport most ESM and customization hook changes #50669

Closed

targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Nov 23, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: avoid try/catch when validating urls #47541

esm: avoid try/catch when validating urls #47541

anonrig commented Apr 13, 2023

nodejs-github-bot commented Apr 13, 2023

aduh95 left a comment •

edited

anonrig commented Apr 13, 2023

aduh95 commented Apr 13, 2023

nodejs-github-bot commented Apr 13, 2023

ljharb commented Apr 13, 2023

anonrig commented Apr 13, 2023

ljharb commented Apr 13, 2023

anonrig commented Apr 13, 2023

JakobJingleheimer left a comment

aduh95 commented Apr 13, 2023

mscdex commented Apr 13, 2023

anonrig commented Apr 13, 2023

ljharb commented Apr 13, 2023

nodejs-github-bot commented Apr 14, 2023

nodejs-github-bot commented Apr 14, 2023

anonrig commented Apr 14, 2023

JakobJingleheimer Apr 15, 2023 •

edited

aduh95 Apr 15, 2023

anonrig Apr 15, 2023

nodejs-github-bot commented Apr 15, 2023

	const { canParse: urlCanParse } = internalBinding('url');
	const { canParse: canParseURL } = internalBinding('url');

esm: avoid try/catch when validating urls #47541

esm: avoid try/catch when validating urls #47541

Conversation

anonrig commented Apr 13, 2023

nodejs-github-bot commented Apr 13, 2023

aduh95 left a comment • edited

Choose a reason for hiding this comment

anonrig commented Apr 13, 2023

aduh95 commented Apr 13, 2023

nodejs-github-bot commented Apr 13, 2023

ljharb commented Apr 13, 2023

anonrig commented Apr 13, 2023

ljharb commented Apr 13, 2023

anonrig commented Apr 13, 2023

JakobJingleheimer left a comment

Choose a reason for hiding this comment

aduh95 commented Apr 13, 2023

mscdex commented Apr 13, 2023

anonrig commented Apr 13, 2023

ljharb commented Apr 13, 2023

nodejs-github-bot commented Apr 14, 2023

nodejs-github-bot commented Apr 14, 2023

anonrig commented Apr 14, 2023

JakobJingleheimer Apr 15, 2023 • edited

Choose a reason for hiding this comment

aduh95 Apr 15, 2023

Choose a reason for hiding this comment

anonrig Apr 15, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 15, 2023

aduh95 left a comment •

edited

JakobJingleheimer Apr 15, 2023 •

edited