Skip to content

Commit

Permalink
perf!: lazy load packageExtensions (#5608)
Browse files Browse the repository at this point in the history
**What's the problem this PR addresses?**
<!-- Describe the rationale of your PR. -->
<!-- Link all issues that it closes. (Closes/Resolves #xxxx.) -->

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?**
<!-- A detailed description of your implementation. -->

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`:
```js
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`
```js
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**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [X] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [X] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [X] I will check that all automated PR checks pass before the PR gets
reviewed.

---------

Co-authored-by: Maël Nison <[email protected]>
  • Loading branch information
paul-soporan and arcanis authored Oct 22, 2023
1 parent 0fdbfe0 commit 207413b
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 16 deletions.
34 changes: 34 additions & 0 deletions .yarn/versions/422a43ec.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
releases:
"@yarnpkg/cli": major
"@yarnpkg/core": major

declined:
- "@yarnpkg/plugin-compat"
- "@yarnpkg/plugin-constraints"
- "@yarnpkg/plugin-dlx"
- "@yarnpkg/plugin-essentials"
- "@yarnpkg/plugin-exec"
- "@yarnpkg/plugin-file"
- "@yarnpkg/plugin-git"
- "@yarnpkg/plugin-github"
- "@yarnpkg/plugin-http"
- "@yarnpkg/plugin-init"
- "@yarnpkg/plugin-interactive-tools"
- "@yarnpkg/plugin-link"
- "@yarnpkg/plugin-nm"
- "@yarnpkg/plugin-npm"
- "@yarnpkg/plugin-npm-cli"
- "@yarnpkg/plugin-pack"
- "@yarnpkg/plugin-patch"
- "@yarnpkg/plugin-pnp"
- "@yarnpkg/plugin-pnpm"
- "@yarnpkg/plugin-stage"
- "@yarnpkg/plugin-typescript"
- "@yarnpkg/plugin-version"
- "@yarnpkg/plugin-workspace-tools"
- "@yarnpkg/builder"
- "@yarnpkg/doctor"
- "@yarnpkg/extensions"
- "@yarnpkg/nm"
- "@yarnpkg/pnpify"
- "@yarnpkg/sdks"
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ The following changes only affect people writing Yarn plugins:
- `workspace.anchoredLocator` to get the locator that's used throughout the dependency tree.
- `workspace.manifest.version` to get the workspace version.

- `configuration.{packageExtensions,refreshPackageExtensions}` have been removed. Use `configuration.getPackageExtensions` instead.

- `configuration.normalizePackage` now requires a `packageExtensions` option.

- `ProjectLookup` has been removed. Both `Configuration.find` and `Configuration.findProjectCwd` now always do a lockfile lookup.

### Installs
Expand Down
27 changes: 16 additions & 11 deletions packages/yarnpkg-core/sources/Configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,8 @@ export interface ConfigurationValueMap {

export type PackageExtensionData = miscUtils.MapValueToObjectValue<miscUtils.MapValue<ConfigurationValueMap['packageExtensions']>>;

export type PackageExtensions = Map<IdentHash, Array<[string, Array<PackageExtension>]>>;

type SimpleDefinitionForType<T> = SimpleSettingsDefinition & {
type:
| (T extends boolean ? SettingsType.BOOLEAN : never)
Expand Down Expand Up @@ -1057,7 +1059,6 @@ export class Configuration {
public invalid: Map<string, string> = new Map();

public env: Record<string, string | undefined> = {};
public packageExtensions: Map<IdentHash, Array<[string, Array<PackageExtension>]>> = new Map();

public limits: Map<string, Limit> = new Map();

Expand Down Expand Up @@ -1357,8 +1358,6 @@ export class Configuration {
configuration.sources.set(`cacheFolder`, `<internal>`);
}

await configuration.refreshPackageExtensions();

return configuration;
}

Expand Down Expand Up @@ -1770,7 +1769,15 @@ export class Configuration {
return {os, cpu, libc};
}

async refreshPackageExtensions() {
private packageExtensions: PackageExtensions | null = null;

/**
* Computes and caches the package extensions.
*/
async getPackageExtensions(): Promise<PackageExtensions> {
if (this.packageExtensions !== null)
return this.packageExtensions;

this.packageExtensions = new Map();
const packageExtensions = this.packageExtensions;

Expand Down Expand Up @@ -1808,9 +1815,10 @@ export class Configuration {
return hooks.registerPackageExtensions;
}, this, registerPackageExtension);

for (const [descriptorString, extensionData] of this.get(`packageExtensions`)) {
for (const [descriptorString, extensionData] of this.get(`packageExtensions`))
registerPackageExtension(structUtils.parseDescriptor(descriptorString, true), miscUtils.convertMapsToIndexableObjects(extensionData), {userProvided: true});
}

return packageExtensions;
}

normalizeLocator(locator: Locator) {
Expand Down Expand Up @@ -1841,16 +1849,13 @@ export class Configuration {
}));
}

normalizePackage(original: Package) {
normalizePackage(original: Package, {packageExtensions}: {packageExtensions: PackageExtensions}) {
const pkg = structUtils.copyPackage(original);

// We use the extensions to define additional dependencies that weren't
// properly listed in the original package definition

if (this.packageExtensions == null)
throw new Error(`refreshPackageExtensions has to be called before normalizing packages`);

const extensionsPerIdent = this.packageExtensions.get(original.identHash);
const extensionsPerIdent = packageExtensions.get(original.identHash);
if (typeof extensionsPerIdent !== `undefined`) {
const version = original.version;

Expand Down
12 changes: 8 additions & 4 deletions packages/yarnpkg-core/sources/Project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,9 @@ export class Project {
}

async preparePackage(originalPkg: Package, {resolver, resolveOptions}: {resolver: Resolver, resolveOptions: ResolveOptions}) {
const pkg = this.configuration.normalizePackage(originalPkg);
const packageExtensions = await this.configuration.getPackageExtensions();

const pkg = this.configuration.normalizePackage(originalPkg, {packageExtensions});

for (const [identHash, descriptor] of pkg.dependencies) {
const dependency = await this.configuration.reduceHook(hooks => {
Expand Down Expand Up @@ -1746,7 +1748,9 @@ export class Project {
if (hasPreErrors)
return;

for (const extensionsByIdent of this.configuration.packageExtensions.values())
const packageExtensions = await this.configuration.getPackageExtensions();

for (const extensionsByIdent of packageExtensions.values())
for (const [, extensionsByRange] of extensionsByIdent)
for (const extension of extensionsByRange)
extension.status = PackageExtensionStatus.Inactive;
Expand Down Expand Up @@ -1776,7 +1780,7 @@ export class Project {
}, async () => {
emitPeerDependencyWarnings(this, opts.report);

for (const [, extensionsPerRange] of this.configuration.packageExtensions) {
for (const [, extensionsPerRange] of packageExtensions) {
for (const [, extensions] of extensionsPerRange) {
for (const extension of extensions) {
if (extension.userProvided) {
Expand Down Expand Up @@ -1827,7 +1831,7 @@ export class Project {
}
});

for (const extensionsByIdent of this.configuration.packageExtensions.values())
for (const extensionsByIdent of packageExtensions.values())
for (const [, extensionsByRange] of extensionsByIdent)
for (const extension of extensionsByRange)
if (extension.userProvided && extension.status === PackageExtensionStatus.Active)
Expand Down
2 changes: 1 addition & 1 deletion packages/yarnpkg-core/sources/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import * as treeUtils from './treeUtils';
export {CACHE_VERSION, CACHE_CHECKPOINT, Cache} from './Cache';
export {DEFAULT_RC_FILENAME, LEGACY_PLUGINS, TAG_REGEXP} from './Configuration';
export {Configuration, FormatType, SettingsType, WindowsLinkType} from './Configuration';
export type {PluginConfiguration, SettingsDefinition, PackageExtensionData} from './Configuration';
export type {PluginConfiguration, SettingsDefinition, PackageExtensionData, PackageExtensions} from './Configuration';
export type {ConfigurationValueMap, ConfigurationDefinitionMap} from './Configuration';
export type {Fetcher, FetchOptions, FetchResult, MinimalFetchOptions} from './Fetcher';
export {BuildDirectiveType} from './Installer';
Expand Down

0 comments on commit 207413b

Please sign in to comment.