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.

Sign up for GitHub

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

Jump to bottom

lib: reduce url getters on makeRequireFunction #48492

Merged
merged 1 commit into from
Jun 24, 2023
Merged

lib: reduce url getters on makeRequireFunction #48492

merged 1 commit into from
Jun 24, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Jun 18, 2023 •
edited
Loading

Reduce calling getters of URL due to the lazy loading of URL.

Ref: nodejs/performance#92

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 18, 2023
GeoffreyBooth
GeoffreyBooth approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Thank you for this!

I gather from this and related PRs that URL objects in general are more costly than primitive strings? Should we make an effort to try to scrub the modules code of URL objects whenever possible, converting to strings instead?

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@anonrig
Copy link
Member Author

anonrig commented Jun 21, 2023

Thank you for this!

I gather from this and related PRs that URL objects in general are more costly than primitive strings? Should we make an effort to try to scrub the modules code of URL objects whenever possible, converting to strings instead?

@GeoffreyBooth URL object is not costly, but accessing their attributes except href is costly. For example, calling url.protocol multiple times, will create multiple different strings, rather than the same one. This is caused by the lazy getters of the class. I'm currently trying to reduce URL initializations and multiple getters of the same URL instance across the loaders.

@GeoffreyBooth
Copy link
Member

For example, calling url.protocol multiple times, will create multiple different strings, rather than the same one.

I would think it should be possible to “cache” protocol in whatever way that href is currently stored? So that calling protocol repeatedly is no more costly than calling href, assuming the latter is “inexpensive” (in the sense that it’s not worth trying to refactor to something even simpler).

@anonrig
Copy link
Member Author

anonrig commented Jun 21, 2023

I would think it should be possible to “cache” protocol in whatever way that href is currently stored? So that calling protocol repeatedly is no more costly than calling href, assuming the latter is “inexpensive” (in the sense that it’s not worth trying to refactor to something even simpler).

This might be an (micro)-optimization worth investigating for, but adding a branch and accessing the cache parameter overhead might be greater than just simply reduce calling it. I simply don't have the data right now to back the claim if one approach is better than another.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 23, 2023
Open
30 tasks
@anonrig
Copy link
Member Author

anonrig commented Jun 23, 2023

Rebased and force-pushed.

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

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

@github-actions github-actions bot mentioned this pull request Jun 24, 2023
Open
25 tasks
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2023
@nodejs-github-bot nodejs-github-bot merged commit 578ffe1 into nodejs:main Jun 24, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 578ffe1

@github-actions github-actions bot mentioned this pull request Jun 25, 2023
Open
25 tasks
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@anonrig @RafaelGSS
lib: reduce url getters on makeRequireFunction
32bda81 
PR-URL: #48492
Refs: nodejs/performance#92
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
lib: reduce url getters on makeRequireFunction
5c6445d 
PR-URL: nodejs#48492
Refs: nodejs/performance#92
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
lib: reduce url getters on makeRequireFunction
cbb7682 
PR-URL: nodejs#48492
Refs: nodejs/performance#92
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 10, 2023
@anonrig @ruyadorno
lib: reduce url getters on makeRequireFunction
edab50c 
PR-URL: #48492
Refs: nodejs/performance#92
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 10, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@anonrig @ruyadorno
lib: reduce url getters on makeRequireFunction
5d682c5 
PR-URL: #48492
Refs: nodejs/performance#92
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@lpinca lpinca lpinca approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@anonrig @nodejs-github-bot @GeoffreyBooth @jasnell @lpinca @JakobJingleheimer @aduh95