Skip to content

lib: improve esm resolve performance#46652

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:perf-improve-esm-resolve
Apr 23, 2023
Merged

lib: improve esm resolve performance#46652
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:perf-improve-esm-resolve

Conversation

@anonrig

@anonrig anonrig commented Feb 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

I couldn't find a benchmark or had the time to write it, but in theory, it should be a lot faster.

cc @nodejs/performance @nodejs/loaders

Refs: nodejs/performance#39

@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Feb 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 14, 2023

Copy link
Copy Markdown
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 Feb 14, 2023
GeoffreyBooth
GeoffreyBooth previously requested changes Feb 14, 2023
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On quick glance this seems fine, but we need to land #44710 first. Just putting a hold on this until that gets landed.

@GeoffreyBooth GeoffreyBooth added the loaders Issues and PRs related to ES module loaders label Feb 14, 2023
targos
targos reviewed Feb 14, 2023
View reviewed changes
Comment thread lib/internal/modules/esm/resolve.js Outdated
@anonrig anonrig force-pushed the perf-improve-esm-resolve branch from d5f008b to 76fb5d6 Compare February 14, 2023 16:38
GeoffreyBooth
GeoffreyBooth reviewed Feb 14, 2023
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I chatted with @anonrig and they offered to rebase #44710 on top of this if this is ready first, which seems fine to me. I’d still like to ensure this doesn’t slow down #44710 landing though; the rebased version of that branch shouldn’t have any new test failures.

@GeoffreyBooth GeoffreyBooth self-requested a review February 14, 2023 16:45
aduh95
aduh95 approved these changes Feb 14, 2023
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd like to see a benchmark for this, but the theory looks solid. This can land before or after the off-thread PR, the conflicts should be minimal :)

@arcanis

arcanis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

Shouldn't it be factorized with the cjs/loader implementation (note that since it isn't in this PR, the esm implementation doesn't has a cache)?

@anonrig

anonrig commented Feb 14, 2023

Copy link
Copy Markdown
Member Author

Shouldn't it be factorized with the cjs/loader implementation (note that since it isn't in this PR, the esm implementation doesn't has a cache)?

I agree, but I prefer to do it in a different pull request, and keep this minimal.

@mscdex mscdex added the needs-citgm PRs that need a CITGM CI run. label Feb 14, 2023
mcollina
mcollina approved these changes Feb 15, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 17, 2023

Copy link
Copy Markdown
Collaborator

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

This was referenced Feb 18, 2023
Open
Open
@nodejs-github-bot

nodejs-github-bot commented Feb 21, 2023

Copy link
Copy Markdown
Collaborator

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

@github-actions github-actions Bot mentioned this pull request Feb 23, 2023
Open
31 tasks
@GeoffreyBooth

GeoffreyBooth commented Mar 13, 2023

Copy link
Copy Markdown
Member

Before we land this shouldn’t there be some attempt at confirming that this does, in fact, make things faster? And especially not slower. Even just running a benchmark on someone’s local machine.

benjamingr
benjamingr previously requested changes Mar 13, 2023
View reviewed changes

@benjamingr benjamingr left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To strengthen @GeoffreyBooth 's point
Making sure benchmarks are run in some form before this lands. Please dismiss my review when that happens @anonrig (or whomever)

@anonrig

anonrig commented Apr 23, 2023 •
edited by aduh95
Loading

Copy link
Copy Markdown
Member Author

@GeoffreyBooth Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1332/

Results 
                                                                              confidence improvement accuracy (*)    (**)   (***)
esm/esm-loader-defaultResolve.js specifier='./relative-existing.js' n=1000             *      9.41 %       ±8.24% ±10.97% ±14.29%
esm/esm-loader-defaultResolve.js specifier='./relative-nonexistent.js' n=1000        ***     15.64 %       ±6.89%  ±9.20% ±12.03%
esm/esm-loader-defaultResolve.js specifier='node:os' n=1000                                  -0.28 %       ±8.78% ±11.68% ±15.20%
esm/esm-loader-defaultResolve.js specifier='node:prefixed-nonexistent' n=1000                -3.20 %       ±7.41%  ±9.87% ±12.85%
esm/esm-loader-defaultResolve.js specifier='unprefixed-existing' n=1000               **     13.37 %       ±8.00% ±10.65% ±13.88%
esm/esm-loader-defaultResolve.js specifier='unprefixed-nonexistent' n=1000             *      9.01 %       ±7.15%  ±9.52% ±12.39%

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 23, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 23, 2023
@nodejs-github-bot nodejs-github-bot merged commit 3e9ed7e into nodejs:main Apr 23, 2023
@nodejs-github-bot

nodejs-github-bot commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator

Landed in 3e9ed7e

This was referenced Apr 24, 2023
Open
Open
Open
Open
@targos targos mentioned this pull request May 2, 2023
Merged
@targos targos mentioned this pull request Nov 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos targos approved these changes

@UlisesGascon UlisesGascon UlisesGascon approved these changes

@aduh95 aduh95 aduh95 approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@ErickWendel ErickWendel Awaiting requested review from ErickWendel

+1 more reviewer

@tony-go tony-go tony-go approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. performance Issues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.