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

perf!: lazy load packageExtensions #5608

Merged
merged 6 commits into from
Oct 22, 2023
Merged

Conversation

paul-soporan
Copy link
Member

@paul-soporan paul-soporan commented Jul 25, 2023

What's the problem this PR addresses?

Loading packageExtensions is currently done every single time Configuration.find is called, and it adds considerable overhead, both on every yarn invocation and even more on commands that don't even use the packageExtensions, such as yarn run.

How did you fix it?

This PR gets rid of the packageExtensions field and the refreshPackageExtensions method in favor of a configuration.getPackageExtensions async method that caches the extensions and is called directly when needing access to packageExtensions.

This change requires configuration.normalizePackage to become async, but that's not a problem since all its callers are async. See review discussion.

Note: packageExtensions can't be a lazy getter because packageExtensions need to be loaded asynchronously.

Benchmarks:

  • yarn -v:
YARN_IGNORE_PATH=1 hyperfine  'node ./before.cjs --version' 'node ./after.cjs --version'
Benchmark 1: node ./before.cjs --version
  Time (mean ± σ):     153.8 ms ±   2.0 ms    [User: 160.5 ms, System: 35.4 ms]
  Range (min  max):   150.7 ms  158.4 ms    19 runs
 
Benchmark 2: node ./after.cjs --version
  Time (mean ± σ):     141.1 ms ±   1.3 ms    [User: 148.4 ms, System: 33.4 ms]
  Range (min  max):   137.1 ms  143.1 ms    21 runs
 
Summary
  'node ./after.cjs --version' ran
    1.09 ± 0.02 times faster than 'node ./before.cjs --version'
  • yarn workspaces foreach run
Benchmark 1: node ./before.cjs workspaces foreach run
  Time (mean ± σ):      5.997 s ±  0.066 s    [User: 7.707 s, System: 1.831 s]
  Range (min  max):    5.870 s   6.072 s    10 runs
 
Benchmark 2: node ./after.cjs workspaces foreach run
  Time (mean ± σ):      5.887 s ±  0.072 s    [User: 7.533 s, System: 1.683 s]
  Range (min  max):    5.769 s   5.977 s    10 runs
 
Summary
  'node ./after.cjs workspaces foreach run' ran
    1.02 ± 0.02 times faster than 'node ./before.cjs workspaces foreach run'

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@paul-soporan paul-soporan mentioned this pull request Jul 25, 2023
13 tasks
@arcanis arcanis merged commit 207413b into master Oct 22, 2023
16 checks passed
@arcanis arcanis deleted the paul/perf/lazy-package-extensions branch October 22, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants