From 8a8a51c394ca0d079f5ee108d5046b5bb65f1aad Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 12 Jan 2026 15:58:25 +0100 Subject: [PATCH] perf: don't calculate package file paths in the store twice (#10428) --- pkg-manager/core/src/install/link.ts | 2 +- pkg-manager/headless/src/index.ts | 2 +- .../headless/src/linkHoistedModules.ts | 2 +- store/cafs-types/src/index.ts | 3 +- store/cafs/src/checkPkgFilesIntegrity.ts | 80 +++++++++++++++++-- store/cafs/src/index.ts | 2 + store/create-cafs-store/src/index.ts | 21 ++--- worker/src/start.ts | 40 ++++------ 8 files changed, 105 insertions(+), 47 deletions(-) diff --git a/pkg-manager/core/src/install/link.ts b/pkg-manager/core/src/install/link.ts index dbd4282061..3fafdc7152 100644 --- a/pkg-manager/core/src/install/link.ts +++ b/pkg-manager/core/src/install/link.ts @@ -463,7 +463,7 @@ async function linkAllPkgs ( depNode.requiresBuild = files.requiresBuild let sideEffectsCacheKey: string | undefined - if (opts.sideEffectsCacheRead && files.sideEffects && !isEmpty(files.sideEffects)) { + if (opts.sideEffectsCacheRead && files.sideEffectsMaps && !isEmpty(files.sideEffectsMaps)) { if (opts?.allowBuild?.(depNode.name, depNode.version) !== false) { sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.depPath, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built diff --git a/pkg-manager/headless/src/index.ts b/pkg-manager/headless/src/index.ts index 37b408201f..d5fa79191f 100644 --- a/pkg-manager/headless/src/index.ts +++ b/pkg-manager/headless/src/index.ts @@ -871,7 +871,7 @@ async function linkAllPkgs ( depNode.requiresBuild = filesResponse.requiresBuild let sideEffectsCacheKey: string | undefined - if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) { + if (opts.sideEffectsCacheRead && filesResponse.sideEffectsMaps && !isEmpty(filesResponse.sideEffectsMaps)) { if (opts?.allowBuild?.(depNode.name, depNode.version) !== false) { sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.dir, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built diff --git a/pkg-manager/headless/src/linkHoistedModules.ts b/pkg-manager/headless/src/linkHoistedModules.ts index 6c9d852f11..2d7762f283 100644 --- a/pkg-manager/headless/src/linkHoistedModules.ts +++ b/pkg-manager/headless/src/linkHoistedModules.ts @@ -114,7 +114,7 @@ async function linkAllPkgsInOrder ( depNode.requiresBuild = filesResponse.requiresBuild let sideEffectsCacheKey: string | undefined - if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) { + if (opts.sideEffectsCacheRead && filesResponse.sideEffectsMaps && !isEmpty(filesResponse.sideEffectsMaps)) { if (opts?.allowBuild?.(depNode.name, depNode.version) !== false) { sideEffectsCacheKey = _calcDepState(dir, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built diff --git a/store/cafs-types/src/index.ts b/store/cafs-types/src/index.ts index 21bc51bcac..71105120bc 100644 --- a/store/cafs-types/src/index.ts +++ b/store/cafs-types/src/index.ts @@ -25,7 +25,8 @@ export interface PackageFilesResponse { resolvedFrom: ResolvedFrom filesMap: FilesMap packageImportMethod?: 'auto' | 'hardlink' | 'copy' | 'clone' | 'clone-or-copy' - sideEffects?: SideEffects + // Pre-calculated file location maps for side effects, avoiding recalculation during import + sideEffectsMaps?: Map requiresBuild: boolean } diff --git a/store/cafs/src/checkPkgFilesIntegrity.ts b/store/cafs/src/checkPkgFilesIntegrity.ts index 56b06a28f0..a1d92978b1 100644 --- a/store/cafs/src/checkPkgFilesIntegrity.ts +++ b/store/cafs/src/checkPkgFilesIntegrity.ts @@ -1,12 +1,13 @@ import fs from 'fs' import util from 'util' -import { type PackageFiles, type PackageFileInfo, type SideEffects } from '@pnpm/cafs-types' +import { type PackageFiles, type PackageFileInfo, type SideEffects, type FilesMap } from '@pnpm/cafs-types' import gfs from '@pnpm/graceful-fs' import { type DependencyManifest } from '@pnpm/types' import rimraf from '@zkochan/rimraf' import ssri from 'ssri' import { getFilePathByModeInCafs } from './getFilePathInCafs.js' import { parseJsonBufferSync } from './parseJson.js' +import { readManifestFromStore } from './readManifestFromStore.js' // We track how many files were checked during installation. // It should be rare that a files content should be checked. @@ -18,6 +19,8 @@ global['verifiedFileIntegrity'] = 0 export interface VerifyResult { passed: boolean manifest?: DependencyManifest + filesMap: FilesMap + sideEffectsMaps?: Map } export interface PackageFilesIndex { @@ -44,21 +47,77 @@ export function checkPkgFilesIntegrity ( const verifiedFilesCache = new Set() const _checkFilesIntegrity = checkFilesIntegrity.bind(null, verifiedFilesCache, storeDir) const verified = _checkFilesIntegrity(pkgIndex.files, readManifest) - if (!verified) return { passed: false } + if (!verified.passed) return verified + + const sideEffectsMaps = new Map() if (pkgIndex.sideEffects) { // We verify all side effects cache. We could optimize it to verify only the side effects cache // that satisfies the current os/arch/platform. // However, it likely won't make a big difference. - for (const [sideEffectName, { added }] of pkgIndex.sideEffects) { + for (const [sideEffectName, { added, deleted }] of pkgIndex.sideEffects) { if (added) { - const { passed } = _checkFilesIntegrity(added) - if (!passed) { + const result = _checkFilesIntegrity(added) + if (!result.passed) { pkgIndex.sideEffects!.delete(sideEffectName) + } else { + sideEffectsMaps.set(sideEffectName, { added: result.filesMap, deleted }) } + } else if (deleted) { + sideEffectsMaps.set(sideEffectName, { deleted }) } } } - return verified + + return { + ...verified, + sideEffectsMaps: sideEffectsMaps.size > 0 ? sideEffectsMaps : undefined, + } +} + +/** + * Builds file maps from package index without verification. + * This is a lightweight alternative to checkPkgFilesIntegrity when verifyStoreIntegrity is disabled. + */ +export function buildFileMapsFromIndex ( + storeDir: string, + pkgIndex: PackageFilesIndex, + readManifest?: boolean +): VerifyResult { + const filesMap: FilesMap = new Map() + + for (const [f, fstat] of pkgIndex.files) { + const filename = getFilePathByModeInCafs(storeDir, fstat.integrity, fstat.mode) + filesMap.set(f, filename) + } + + const sideEffectsMaps = new Map() + if (pkgIndex.sideEffects) { + for (const [sideEffectName, { added, deleted }] of pkgIndex.sideEffects) { + const sideEffectEntry: { added?: FilesMap, deleted?: string[] } = {} + + if (added) { + const addedFilesMap: FilesMap = new Map() + for (const [f, fstat] of added) { + const filename = getFilePathByModeInCafs(storeDir, fstat.integrity, fstat.mode) + addedFilesMap.set(f, filename) + } + sideEffectEntry.added = addedFilesMap + } + + if (deleted) { + sideEffectEntry.deleted = deleted + } + + sideEffectsMaps.set(sideEffectName, sideEffectEntry) + } + } + + return { + passed: true, + manifest: readManifest ? readManifestFromStore(storeDir, pkgIndex) : undefined, + filesMap, + sideEffectsMaps: sideEffectsMaps.size > 0 ? sideEffectsMaps : undefined, + } } function checkFilesIntegrity ( @@ -69,11 +128,15 @@ function checkFilesIntegrity ( ): VerifyResult { let allVerified = true let manifest: DependencyManifest | undefined + const filesMap: FilesMap = new Map() + for (const [f, fstat] of files) { if (!fstat.integrity) { throw new Error(`Integrity checksum is missing for ${f}`) } const filename = getFilePathByModeInCafs(storeDir, fstat.integrity, fstat.mode) + filesMap.set(f, filename) + const readFile = readManifest && f === 'package.json' if (!readFile && verifiedFilesCache.has(filename)) continue const verifyResult = verifyFile(filename, fstat, readFile) @@ -89,6 +152,7 @@ function checkFilesIntegrity ( return { passed: allVerified, manifest, + filesMap, } } @@ -100,7 +164,7 @@ function verifyFile ( filename: string, fstat: FileInfo, readManifest?: boolean -): VerifyResult { +): Pick { const currentFile = checkFile(filename, fstat.checkedAt) if (currentFile == null) return { passed: false } if (currentFile.isModified) { @@ -125,7 +189,7 @@ export function verifyFileIntegrity ( filename: string, expectedFile: FileInfo, readManifest?: boolean -): VerifyResult { +): Pick { // @ts-expect-error global['verifiedFileIntegrity']++ try { diff --git a/store/cafs/src/index.ts b/store/cafs/src/index.ts index 008bc0ef27..39a69275b0 100644 --- a/store/cafs/src/index.ts +++ b/store/cafs/src/index.ts @@ -4,6 +4,7 @@ import { addFilesFromDir } from './addFilesFromDir.js' import { addFilesFromTarball } from './addFilesFromTarball.js' import { checkPkgFilesIntegrity, + buildFileMapsFromIndex, type PackageFilesIndex, type VerifyResult, } from './checkPkgFilesIntegrity.js' @@ -21,6 +22,7 @@ export type { IntegrityLike } from 'ssri' export { checkPkgFilesIntegrity, + buildFileMapsFromIndex, readManifestFromStore, type FileType, getFilePathByModeInCafs, diff --git a/store/create-cafs-store/src/index.ts b/store/create-cafs-store/src/index.ts index 08bc496fbf..e1c9867516 100644 --- a/store/create-cafs-store/src/index.ts +++ b/store/create-cafs-store/src/index.ts @@ -3,9 +3,8 @@ import path from 'path' import { type CafsLocker, createCafs, - getFilePathByModeInCafs, } from '@pnpm/store.cafs' -import { type Cafs, type PackageFilesResponse, type SideEffectsDiff, type FilesMap } from '@pnpm/cafs-types' +import { type Cafs, type PackageFilesResponse, type FilesMap } from '@pnpm/cafs-types' import { createIndexedPkgImporter } from '@pnpm/fs.indexed-pkg-importer' import { type ImportIndexedPackage, @@ -83,10 +82,11 @@ function getFlatMap ( filesResponse: PackageFilesResponse, targetEngine?: string ): { filesMap: FilesMap, isBuilt: boolean } { - if (targetEngine && filesResponse.sideEffects?.has(targetEngine)) { - const filesIndexWithSideEffects = applySideEffectsDiff(storeDir, filesResponse.filesMap, filesResponse.sideEffects.get(targetEngine)!) + if (targetEngine && filesResponse.sideEffectsMaps?.has(targetEngine)) { + const sideEffectMap = filesResponse.sideEffectsMaps.get(targetEngine)! + const filesMap = applySideEffectsDiffWithMaps(filesResponse.filesMap, sideEffectMap) return { - filesMap: filesIndexWithSideEffects, + filesMap, isBuilt: true, } } @@ -96,12 +96,15 @@ function getFlatMap ( } } -function applySideEffectsDiff (storeDir: string, baseFiles: FilesMap, { added, deleted }: SideEffectsDiff): FilesMap { +// Apply side effects when we already have file location maps (fast path) +function applySideEffectsDiffWithMaps ( + baseFiles: FilesMap, + { added, deleted }: { added?: FilesMap, deleted?: string[] } +): FilesMap { const filesWithSideEffects = new Map() - // Add side effect files (convert from PackageFiles metadata to file paths) + // Add side effect files (already have file paths) if (added) { - for (const [name, fileInfo] of added.entries()) { - const filePath = getFilePathByModeInCafs(storeDir, fileInfo.integrity, fileInfo.mode) + for (const [name, filePath] of added.entries()) { filesWithSideEffects.set(name, filePath) } } diff --git a/worker/src/start.ts b/worker/src/start.ts index dfeba69201..ed415e4158 100644 --- a/worker/src/start.ts +++ b/worker/src/start.ts @@ -12,11 +12,11 @@ import { hardLinkDir } from '@pnpm/fs.hard-link-dir' import { type CafsFunctions, checkPkgFilesIntegrity, + buildFileMapsFromIndex, createCafs, type PackageFilesIndex, type FilesIndex, optimisticRenameOverwrite, - readManifestFromStore, type VerifyResult, } from '@pnpm/store.cafs' import { symlinkDependencySync } from '@pnpm/symlink-dependency' @@ -100,20 +100,6 @@ async function handleMessage ( }) return } - let verifyResult: VerifyResult | undefined - if (pkgFilesIndex.requiresBuild == null) { - readManifest = true - } - if (verifyStoreIntegrity) { - verifyResult = checkPkgFilesIntegrity(storeDir, pkgFilesIndex, readManifest) - } else { - verifyResult = { - passed: true, - manifest: readManifest ? readManifestFromStore(storeDir, pkgFilesIndex) : undefined, - } - } - const requiresBuild = pkgFilesIndex.requiresBuild ?? pkgRequiresBuild(verifyResult.manifest, pkgFilesIndex.files) - const warnings: string[] = [] if (expectedPkg) { if ( @@ -139,16 +125,18 @@ async function handleMessage ( } } } + let verifyResult: VerifyResult | undefined + if (pkgFilesIndex.requiresBuild == null) { + readManifest = true + } + // Get file maps and optionally verify + if (verifyStoreIntegrity) { + verifyResult = checkPkgFilesIntegrity(storeDir, pkgFilesIndex, readManifest) + } else { + verifyResult = buildFileMapsFromIndex(storeDir, pkgFilesIndex, readManifest) + } + const requiresBuild = pkgFilesIndex.requiresBuild ?? pkgRequiresBuild(verifyResult.manifest, pkgFilesIndex.files) - // Convert PackageFiles to Map - if (!cafsCache.has(storeDir)) { - cafsCache.set(storeDir, createCafs(storeDir)) - } - const cafs = cafsCache.get(storeDir)! - const filesMap: FilesMap = new Map() - for (const [filename, fileInfo] of pkgFilesIndex.files) { - filesMap.set(filename, cafs.getFilePathByModeInCafs(fileInfo.integrity, fileInfo.mode)) - } parentPort!.postMessage({ status: 'success', warnings, @@ -156,8 +144,8 @@ async function handleMessage ( verified: verifyResult.passed, manifest: verifyResult.manifest, files: { - filesMap, - sideEffects: pkgFilesIndex.sideEffects, + filesMap: verifyResult.filesMap, + sideEffectsMaps: verifyResult.sideEffectsMaps, resolvedFrom: 'store', requiresBuild, },