diff --git a/.changeset/ninety-poets-shake.md b/.changeset/ninety-poets-shake.md new file mode 100644 index 0000000000..dbbd017745 --- /dev/null +++ b/.changeset/ninety-poets-shake.md @@ -0,0 +1,5 @@ +--- +"@pnpm/package-requester": minor +--- + +In some cases, a filtered install (i.e. `pnpm install --filter ...`) was slower than running `pnpm install` without any filter arguments. This performance regression is now fixed. Filtered installs should be as fast or faster than a full install. diff --git a/pkg-manager/package-requester/src/packageRequester.ts b/pkg-manager/package-requester/src/packageRequester.ts index d8b3e25adc..2e9d41e7c0 100644 --- a/pkg-manager/package-requester/src/packageRequester.ts +++ b/pkg-manager/package-requester/src/packageRequester.ts @@ -14,7 +14,7 @@ import { type FetchOptions, type FetchResult, } from '@pnpm/fetcher-base' -import { type Cafs } from '@pnpm/cafs-types' +import { type Cafs, type PackageFilesResponse } from '@pnpm/cafs-types' import gfs from '@pnpm/graceful-fs' import { globalWarn, logger } from '@pnpm/logger' import { packageIsInstallable } from '@pnpm/package-is-installable' @@ -119,11 +119,13 @@ export function createPackageRequester ( const getIndexFilePathInCafs = _getIndexFilePathInCafs.bind(null, opts.storeDir) const fetch = fetcher.bind(null, opts.fetchers, opts.cafs, opts.customFetchers) + const getFilePathByModeInCafs = _getFilePathByModeInCafs.bind(null, opts.storeDir) + const readPkgFromCafs = _readPkgFromCafs.bind(null, opts.storeDir, opts.verifyStoreIntegrity) const fetchPackageToStore = fetchToStore.bind(null, { - readPkgFromCafs: _readPkgFromCafs.bind(null, opts.storeDir, opts.verifyStoreIntegrity), + readPkgFromCafs, fetch, fetchingLocker: new Map(), - getFilePathByModeInCafs: _getFilePathByModeInCafs.bind(null, opts.storeDir), + getFilePathByModeInCafs, getIndexFilePathInCafs, requestsQueue: Object.assign(requestsQueue, { counter: 0, @@ -133,12 +135,19 @@ export function createPackageRequester ( virtualStoreDirMaxLength: opts.virtualStoreDirMaxLength, strictStorePkgContentCheck: opts.strictStorePkgContentCheck, }) + const peekPackageFromStore = peekFromStore.bind(null, { + getIndexFilePathInCafs, + readPkgFromCafs, + fetchingLockerForPeek: new Map(), + strictStorePkgContentCheck: opts.strictStorePkgContentCheck, + }) const requestPackage = resolveAndFetch.bind(null, { engineStrict: opts.engineStrict, nodeVersion: opts.nodeVersion, pnpmVersion: opts.pnpmVersion, force: opts.force, fetchPackageToStore, + peekPackageFromStore, requestsQueue, resolve: opts.resolve, storeDir: opts.storeDir, @@ -164,6 +173,10 @@ async function resolveAndFetch ( requestsQueue: { add: (fn: () => Promise, opts: { priority: number }) => Promise } resolve: ResolveFunction fetchPackageToStore: FetchPackageToStoreFunction + peekPackageFromStore: (pkg: PkgNameVersion & { + id: string + resolution: Resolution + }) => Promise storeDir: string }, wantedDependency: WantedDependency & { optional?: boolean }, @@ -181,12 +194,7 @@ async function resolveAndFetch ( let resolvedVia: string | undefined let publishedAt: string | undefined - // When fetching is skipped, resolution cannot be skipped. - // We need the package's manifest when doing `lockfile-only` installs. - // When we don't fetch, the only way to get the package's manifest is via resolving it. - // - // The resolution step is never skipped for local dependencies. - if (!skipResolution || options.skipFetch === true || Boolean(pkgId?.startsWith('file:')) || wantedDependency.optional === true) { + async function performResolution () { // When skipResolution is set but a resolution is still performed due to // options.skipFetch, it's necessary to make sure the resolution doesn't // accidentally return a newer version of the package. When skipFetch is @@ -244,6 +252,35 @@ async function resolveAndFetch ( alias = resolveResult.alias } + // When fetching is skipped, we still need the package's manifest for + // lockfile-only installs. + // + // The package manifest could be retrieved from CAFS if the package has been + // fetched previously. Otherwise we'll need to perform a resolution. + // + // The resolution step is never skipped for local dependencies. + if (!skipResolution || options.skipFetch === true || Boolean(pkgId?.startsWith('file:')) || wantedDependency.optional === true) { + // If the manifest for this package is in the store, retrieving it from the + // Content-Addressable File Store will be significantly faster than fetching + // it from metadata. This is because package metadata contains the + // package.json contents of a package across all of its versions, which + // takes a long time to parse. + if (skipResolution && !pkgId?.startsWith('file:') && wantedDependency.optional !== true && pkgId != null) { + const pkg = await ctx.peekPackageFromStore({ + ...options.expectedPkg, + id: pkgId, + resolution, + }) + manifest = pkg?.bundledManifest + } + + // However, if the optimization above isn't possible or the package has + // never been added to the CAFS, we'll need to perform a resolution. + if (manifest == null) { + await performResolution() + } + } + const id = pkgId! if ('type' in resolution && resolution.type === 'directory' && !id.startsWith('file:')) { @@ -564,34 +601,7 @@ function fetchToStore ( ) { const { verified, pkgFilesIndex, manifest, requiresBuild } = await ctx.readPkgFromCafs(filesIndexFile, opts.fetchRawManifest) if (verified) { - if ( - ( - pkgFilesIndex.name != null && - opts.pkg?.name != null && - pkgFilesIndex.name.toLowerCase() !== opts.pkg.name.toLowerCase() - ) || - ( - pkgFilesIndex.version != null && - opts.pkg?.version != null && - // We used to not normalize the package versions before writing them to the lockfile and store. - // So it may happen that the version will be in different formats. - // For instance, v1.0.0 and 1.0.0 - // Hence, we need to use semver.eq() to compare them. - !equalOrSemverEqual(pkgFilesIndex.version, opts.pkg.version) - ) - ) { - const msg = `Package name mismatch found while reading ${JSON.stringify(opts.pkg.resolution)} from the store.` - const hint = `This means that either the lockfile is broken or the package metadata (name and version) inside the package's package.json file doesn't match the metadata in the registry. \ -Expected package: ${opts.pkg.name}@${opts.pkg.version}. \ -Actual package in the store with the given integrity: ${pkgFilesIndex.name}@${pkgFilesIndex.version}.` - if (ctx.strictStorePkgContentCheck ?? true) { - throw new PnpmError('UNEXPECTED_PKG_CONTENT_IN_STORE', msg, { - hint: `${hint}\n\nIf you want to ignore this issue, set the strict-store-pkg-content-check to false.`, - }) - } else { - globalWarn(`${msg} ${hint}`) - } - } + checkPackageMismatch({ pkgFilesIndex, pkg: opts.pkg, strictStorePkgContentCheck: ctx.strictStorePkgContentCheck }) fetching.resolve({ files: { unprocessed: true, @@ -673,10 +683,119 @@ Actual package in the store with the given integrity: ${pkgFilesIndex.name}@${pk } } +interface PeekFromStoreResult { + readonly bundledManifest?: BundledManifest + readonly files: PackageFilesResponse +} + +/** + * Look up requests from the store without mutating it. This is related to + * {@link fetchToStore}, but does not perform a fetch if the lookup was not + * found. + * + * This function will return undefined if the package was found in the store, + * but fails integrity checks. + */ +async function peekFromStore ( + ctx: { + readPkgFromCafs: ( + filesIndexFile: string, + readManifest?: boolean + ) => Promise<{ verified: boolean, pkgFilesIndex: PackageFilesIndex, manifest?: DependencyManifest, requiresBuild: boolean }> + getIndexFilePathInCafs: (integrity: string, pkgId: string) => string + fetchingLockerForPeek: Map> + strictStorePkgContentCheck: boolean | undefined + }, + pkg: PkgNameVersion & { + id: string + resolution: Resolution + } +): Promise { + // This function only supports TarballResolution requests with a present + // integrity. + if (pkg.resolution.type != null || pkg.resolution.integrity == null) { + return undefined + } + + const indexFilePathInCafs = ctx.getIndexFilePathInCafs(pkg.resolution.integrity, pkg.id) + const existingRequest = ctx.fetchingLockerForPeek.get(indexFilePathInCafs) + + if (existingRequest != null) { + return existingRequest + } + + const request = ctx.readPkgFromCafs(indexFilePathInCafs, true) + .then(({ pkgFilesIndex, manifest, requiresBuild, verified }): PeekFromStoreResult | undefined => { + // If the files in the store are corrupted or out of date, it's better to + // fail the peek result and allow the caller to fall back to a resolution + // or proper fetch. + if (!verified) { + return undefined + } + + checkPackageMismatch({ pkgFilesIndex, pkg, strictStorePkgContentCheck: ctx.strictStorePkgContentCheck }) + + return { + files: { + unprocessed: true, + resolvedFrom: 'store', + filesIndex: pkgFilesIndex.files, + sideEffects: pkgFilesIndex.sideEffects, + requiresBuild, + }, + bundledManifest: manifest == null ? manifest : normalizeBundledManifest(manifest), + } + }) + + ctx.fetchingLockerForPeek.set(indexFilePathInCafs, request) + + return request +} + async function readBundledManifest (pkgJsonPath: string): Promise { return pickBundledManifest(await readPackageJson(pkgJsonPath) as DependencyManifest) } +function checkPackageMismatch ( + { pkgFilesIndex, pkg, strictStorePkgContentCheck }: { + pkgFilesIndex: PackageFilesIndex + pkg?: PkgNameVersion & { + id: string + resolution: Resolution + } + strictStorePkgContentCheck: boolean | undefined + } +) { + if ( + ( + pkgFilesIndex.name != null && + pkg?.name != null && + pkgFilesIndex.name.toLowerCase() !== pkg.name.toLowerCase() + ) || + ( + pkgFilesIndex.version != null && + pkg?.version != null && + // We used to not normalize the package versions before writing them to the lockfile and store. + // So it may happen that the version will be in different formats. + // For instance, v1.0.0 and 1.0.0 + // Hence, we need to use semver.eq() to compare them. + !equalOrSemverEqual(pkgFilesIndex.version, pkg.version) + ) + ) { + const msg = `Package name mismatch found while reading ${JSON.stringify(pkg.resolution)} from the store.` + const hint = `This means that either the lockfile is broken or the package metadata (name and version) inside the package's package.json file doesn't match the metadata in the registry. \ +Expected package: ${pkg.name}@${pkg.version}. \ +Actual package in the store with the given integrity: ${pkgFilesIndex.name}@${pkgFilesIndex.version}.` + if (strictStorePkgContentCheck ?? true) { + throw new PnpmError('UNEXPECTED_PKG_CONTENT_IN_STORE', msg, { + hint: `${hint}\n\nIf you want to ignore this issue, set the strict-store-pkg-content-check to false.`, + }) + } else { + globalWarn(`${msg} ${hint}`) + } + } +} + async function tarballIsUpToDate ( resolution: { integrity?: string diff --git a/pkg-manager/package-requester/test/index.ts b/pkg-manager/package-requester/test/index.ts index 17db79740b..cb4ceca2fc 100644 --- a/pkg-manager/package-requester/test/index.ts +++ b/pkg-manager/package-requester/test/index.ts @@ -75,7 +75,7 @@ test('request package', async () => { }) test('request package but skip fetching', async () => { - const storeDir = '.store' + const storeDir = temporaryDirectory() const cafs = createCafsStore(storeDir) const requestPackage = createPackageRequester({ resolve, @@ -114,7 +114,7 @@ test('request package but skip fetching', async () => { }) test('request package but skip fetching, when resolution is already available', async () => { - const storeDir = '.store' + const storeDir = temporaryDirectory() const cafs = createCafsStore(storeDir) const requestPackage = createPackageRequester({ resolve, @@ -165,6 +165,80 @@ test('request package but skip fetching, when resolution is already available', expect(pkgResponse.fetching).toBeFalsy() }) +test('request package but skip fetching, when resolution is already available and manifest is in store', async () => { + const storeDir = temporaryDirectory() + const cafs = createCafsStore(storeDir) + const resolveMockFn = jest.fn(resolve) + const projectDir = temporaryDirectory() + + const createPackageRequesterOptions = { + resolve: resolveMockFn, + fetchers, + cafs, + networkConcurrency: 1, + storeDir, + verifyStoreIntegrity: true, + virtualStoreDirMaxLength: 120, + } + const requestPackage = createPackageRequester(createPackageRequesterOptions) + + const requestOpts: RequestPackageOptions = { + currentPkg: { + id: 'is-positive@1.0.0' as PkgResolutionId, + resolution: { + integrity: 'sha512-xxzPGZ4P2uN6rROUa5N9Z7zTX6ERuE0hs6GUOc/cKBLF2NqKc16UwqHMt3tFg4CO6EBTE5UecUasg+3jZx3Ckg==', + tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`, + }, + }, + downloadPriority: 0, + lockfileDir: projectDir, + preferredVersions: {}, + projectDir, + update: false, + } + + // Check that the test setup in this function is correct by performing a + // preliminary request with the skipFetch option. + // + // Since the store has not yet been populated, the package manifest for this + // function will need to be retrieved through a resolve call. + await requestPackage({ alias: 'is-positive', bareSpecifier: '1.0.0' }, { ...requestOpts, skipFetch: true }) + expect(resolveMockFn).toHaveBeenCalledTimes(1) + + // Perform a request without skipFetch to populate the CAFS store. + await requestPackage({ alias: 'is-positive', bareSpecifier: '1.0.0' }, requestOpts) + + // The final request should reuse the package manifest present in the CAFS + // store. The resolve function should not be called. If it is called, the + // optimization this test is checking for regressed. + // + // We need to create a new package request function for this test to reset the + // fetching lockers used internally within the package requester function. + // Otherwise the requestPackage function will use a prior cached store lookup + // that resolved to undefined. + resolveMockFn.mockClear() + const requestPackage2 = createPackageRequester(createPackageRequesterOptions) + const pkgResponse = await requestPackage2({ alias: 'is-positive', bareSpecifier: '1.0.0' }, { ...requestOpts, skipFetch: true }) + expect(resolveMockFn).not.toHaveBeenCalled() + + expect(pkgResponse).toBeTruthy() + expect(pkgResponse.body).toBeTruthy() + + // Since the package wasn't resolved, the resolvedVia field should be undefined. + expect(pkgResponse.body.resolvedVia).toBeUndefined() + + expect(pkgResponse.body.id).toBe('is-positive@1.0.0') + expect(pkgResponse.body.isLocal).toBe(false) + expect(pkgResponse.body.manifest?.name).toBe('is-positive') + expect(!pkgResponse.body.normalizedBareSpecifier).toBeTruthy() + expect(pkgResponse.body.resolution).toStrictEqual({ + integrity: 'sha512-xxzPGZ4P2uN6rROUa5N9Z7zTX6ERuE0hs6GUOc/cKBLF2NqKc16UwqHMt3tFg4CO6EBTE5UecUasg+3jZx3Ckg==', + tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`, + }) + + expect(pkgResponse.fetching).toBeFalsy() +}) + test('refetch local tarball if its integrity has changed', async () => { const projectDir = temporaryDirectory() const tarballPath = path.join(projectDir, 'tarball.tgz') @@ -753,7 +827,7 @@ test('refetch package to store if it has been modified', async () => { }) test('do not fetch an optional package that is not installable', async () => { - const storeDir = '.store' + const storeDir = temporaryDirectory() const cafs = createCafsStore(storeDir) const requestPackage = createPackageRequester({ resolve,