esm: avoid try/catch when validating urls by anonrig · Pull Request #47541 · nodejs/node · GitHub

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

Review requested:

@nodejs/loaders
@nodejs/modules

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?

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?

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 :)

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

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

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.

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

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?

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

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.

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?

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

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

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

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

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

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.

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 outstanding review comments, 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 JakobJingleheimer, aduh95 and lpinca 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

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 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 and removed backport-requested-v18.x labels Nov 23, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

               } = require('internal/errors').codes;
               const { exitCodes: { kUnfinishedTopLevelAwait } } = internalBinding('errors');
               const { URL } = require('internal/url');
+              const { canParse: urlCanParse } = internalBinding('url');

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

esm: avoid try/catch when validating urls by anonrig · Pull Request #47541 · nodejs/node · GitHub

Uh oh!

Uh oh!

esm: avoid try/catch when validating urls - #47541

esm: avoid try/catch when validating urls#47541nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom anonrig:improve-esm-performanceanonrig/node:improve-esm-performanceCopy head branch name to clipboard

Conversation

anonrig commented Apr 13, 2023

Uh oh!

nodejs-github-bot commented Apr 13, 2023

Uh oh!

aduh95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig commented Apr 13, 2023

Uh oh!

aduh95 commented Apr 13, 2023

Uh oh!

nodejs-github-bot commented Apr 13, 2023

Uh oh!

ljharb commented Apr 13, 2023

Uh oh!

anonrig commented Apr 13, 2023

Uh oh!

ljharb commented Apr 13, 2023

Uh oh!

anonrig commented Apr 13, 2023

Uh oh!

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Apr 13, 2023

Uh oh!

mscdex commented Apr 13, 2023

Uh oh!

anonrig commented Apr 13, 2023

Uh oh!

ljharb commented Apr 13, 2023

Uh oh!

nodejs-github-bot commented Apr 14, 2023

Uh oh!

nodejs-github-bot commented Apr 14, 2023

Uh oh!

anonrig commented Apr 14, 2023

Uh oh!

JakobJingleheimer Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

anonrig Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

esm: avoid try/catch when validating urls#47541
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:improve-esm-performance

aduh95 left a comment •
edited
Loading

JakobJingleheimer Apr 15, 2023 •
edited
Loading