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

esm: avoid try/catch when validating urls #47541

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Apr 13, 2023

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

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

cc @nodejs/url

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot 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
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

anonrig commented Apr 13, 2023

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
Copy link
Contributor

aduh95 commented Apr 13, 2023

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

@anonrig 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@ljharb
Copy link
Member

ljharb commented Apr 13, 2023

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

@anonrig
Copy link
Member Author

anonrig commented Apr 13, 2023

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
Copy link
Member

ljharb commented Apr 13, 2023

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

@anonrig
Copy link
Member Author

anonrig commented Apr 13, 2023

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?

Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

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

@aduh95
Copy link
Contributor

aduh95 commented Apr 13, 2023

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
Copy link
Contributor

mscdex commented Apr 13, 2023

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
Copy link
Member Author

anonrig commented Apr 13, 2023

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
Copy link
Member

ljharb commented Apr 13, 2023

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

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member Author

anonrig commented Apr 14, 2023

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

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 14, 2023
@@ -35,6 +35,7 @@ const {
} = require('internal/errors').codes;
const { exitCodes: { kUnfinishedTopLevelAwait } } = internalBinding('errors');
const { URL } = require('internal/url');
const { canParse: urlCanParse } = internalBinding('url');
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer Apr 15, 2023

Choose a reason for hiding this comment

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

nit: this sounds a little Yoda-esque

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 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 nodejs-github-bot merged commit 17570c0 into nodejs:main Apr 15, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 17570c0

targos pushed a commit that referenced this pull request May 2, 2023
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>
@targos targos mentioned this pull request May 2, 2023
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 3, 2023
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
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>
@targos 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 pushed a commit that referenced this pull request Nov 23, 2023
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>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants