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: don't use await inside loops #6617

Merged
merged 12 commits into from Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions .changeset/kind-berries-wave.md
@@ -0,0 +1,17 @@
---
"@pnpm/plugin-commands-publishing": patch
"@pnpm/plugin-commands-patching": patch
"@pnpm/plugin-commands-listing": patch
"@pnpm/resolve-dependencies": patch
"@pnpm/exportable-manifest": patch
"@pnpm/license-scanner": patch
"@pnpm/get-context": patch
"@pnpm/headless": patch
"@pnpm/read-modules-dir": patch
"@pnpm/package-store": patch
"@pnpm/core": patch
"@pnpm/audit": patch
"@pnpm/list": patch
---

Don't use await in loops.
5 changes: 5 additions & 0 deletions .changeset/stale-cups-compete.md
@@ -0,0 +1,5 @@
---
"pnpm": patch
---

Some minor performance improvements by removing await from loops [#6617](https://github.com/pnpm/pnpm/pull/6617).
1 change: 1 addition & 0 deletions __utils__/eslint-config/eslint.json
Expand Up @@ -34,6 +34,7 @@
"@typescript-eslint/explicit-function-return-type": "off",
"@typescript-eslint/no-explicit-any": "error",
"no-return-await": "error",
"no-await-in-loop": "error",
"@typescript-eslint/return-await": "off",
"@typescript-eslint/no-require-imports": "error",
"@typescript-eslint/no-unused-expressions": "error",
Expand Down
1 change: 1 addition & 0 deletions cli/parse-cli-args/test/index.ts
Expand Up @@ -244,6 +244,7 @@ test.each([
// shouldn't affect its arg parsing. Test both scenarios for good measure.
const input = [...(testWithCommandFallback ? [] : ['run']), ...testInput.split(' ')]

// eslint-disable-next-line no-await-in-loop
const { options, cmd, params, fallbackCommandUsed } = await parseCliArgs({
...DEFAULT_OPTS,
fallbackCommand: 'run',
Expand Down
1 change: 1 addition & 0 deletions env/plugin-commands-env/test/env.test.ts
Expand Up @@ -119,6 +119,7 @@ test('it re-attempts failed downloads', async () => {
try {
const attempts = 2
for (let i = 0; i < attempts; i++) {
// eslint-disable-next-line no-await-in-loop
await expect(
env.handler({
bin: process.cwd(),
Expand Down
2 changes: 1 addition & 1 deletion exec/lifecycle/src/runLifecycleHooksConcurrently.ts
Expand Up @@ -52,7 +52,7 @@ export async function runLifecycleHooksConcurrently (
let isBuilt = false
for (const stage of (importerStages ?? stages)) {
if ((manifest.scripts == null) || !manifest.scripts[stage]) continue
await runLifecycleHook(stage, manifest, runLifecycleHookOpts)
await runLifecycleHook(stage, manifest, runLifecycleHookOpts) // eslint-disable-line no-await-in-loop
isBuilt = true
}
if (targetDirs == null || targetDirs.length === 0 || !isBuilt) return
Expand Down
1 change: 1 addition & 0 deletions exec/plugin-commands-rebuild/src/recursive.ts
Expand Up @@ -111,6 +111,7 @@ export async function recursiveRebuild (
}
const limitRebuild = pLimit(opts.workspaceConcurrency ?? 4)
for (const chunk of chunks) {
// eslint-disable-next-line no-await-in-loop
await Promise.all(chunk.map(async (rootDir: string) =>
limitRebuild(async () => {
try {
Expand Down
1 change: 1 addition & 0 deletions exec/plugin-commands-script-runners/src/exec.ts
Expand Up @@ -180,6 +180,7 @@ export async function handler (

let exitCode = 0
for (const chunk of chunks) {
// eslint-disable-next-line no-await-in-loop
await Promise.all(chunk.map(async (prefix: string) =>
limitRun(async () => {
result[prefix].status = 'running'
Expand Down
1 change: 1 addition & 0 deletions exec/plugin-commands-script-runners/src/runRecursive.ts
Expand Up @@ -90,6 +90,7 @@ export async function runRecursive (
return specifiedScripts.map(script => ({ prefix, scriptName: script }))
}).flat()

// eslint-disable-next-line no-await-in-loop
await Promise.all(selectedScripts.map(async ({ prefix, scriptName }) =>
limitRun(async () => {
const pkg = opts.selectedProjectsGraph[prefix]
Expand Down
1 change: 1 addition & 0 deletions exec/prepare-package/src/index.ts
Expand Up @@ -41,6 +41,7 @@ export async function preparePackage (opts: PreparePackageOptions, pkgDir: strin
await runLifecycleHook(installScriptName, manifest, execOpts)
for (const scriptName of PREPUBLISH_SCRIPTS) {
if (manifest.scripts[scriptName] == null || manifest.scripts[scriptName] === '') continue
// eslint-disable-next-line no-await-in-loop
await runLifecycleHook(scriptName, manifest, execOpts)
}
} catch (err: any) { // eslint-disable-line
Expand Down
15 changes: 6 additions & 9 deletions fs/read-modules-dir/src/index.ts
Expand Up @@ -17,21 +17,18 @@ async function _readModulesDir (
modulesDir: string,
scope?: string
) {
let pkgNames: string[] = []
const pkgNames: string[] = []
const parentDir = scope ? path.join(modulesDir, scope) : modulesDir
for (const dir of await readdir(parentDir, { withFileTypes: true })) {
if (dir.isFile() || dir.name[0] === '.') continue
await Promise.all((await readdir(parentDir, { withFileTypes: true })).map(async (dir) => {
if (dir.isFile() || dir.name[0] === '.') return

if (!scope && dir.name[0] === '@') {
pkgNames = [
...pkgNames,
...await _readModulesDir(modulesDir, dir.name),
]
continue
pkgNames.push(...await _readModulesDir(modulesDir, dir.name))
return
}

const pkgName = scope ? `${scope}/${dir.name as string}` : dir.name
pkgNames.push(pkgName)
}
}))
return pkgNames
}
11 changes: 6 additions & 5 deletions lockfile/audit/src/index.ts
Expand Up @@ -83,11 +83,12 @@ async function extendWithDependencyPaths (auditReport: AuditReport, opts: {
include: opts.include,
}
const _searchPackagePaths = searchPackagePaths.bind(null, searchOpts, projectDirs)
for (const { findings, module_name: moduleName } of Object.values(advisories)) {
for (const finding of findings) {
finding.paths = await _searchPackagePaths(`${moduleName}@${finding.version}`)
}
}
// eslint-disable-next-line @typescript-eslint/naming-convention
await Promise.all(Object.values(advisories).map(async ({ findings, module_name }) => {
await Promise.all(findings.map(async (finding) => {
finding.paths = await _searchPackagePaths(`${module_name}@${finding.version}`)
}))
}))
return auditReport
}

Expand Down
2 changes: 2 additions & 0 deletions lockfile/lockfile-file/src/read.ts
Expand Up @@ -178,6 +178,7 @@ async function _readWantedLockfile (
}
}
let result: { lockfile: Lockfile | null, hadConflicts: boolean } = { lockfile: null, hadConflicts: false }
/* eslint-disable no-await-in-loop */
for (const lockfileName of lockfileNames) {
result = await _read(path.join(pkgPath, lockfileName), pkgPath, { ...opts, autofixMergeConflicts: true })
if (result.lockfile) {
Expand All @@ -187,6 +188,7 @@ async function _readWantedLockfile (
break
}
}
/* eslint-enable no-await-in-loop */
return result
}

Expand Down
2 changes: 2 additions & 0 deletions network/fetch/src/fetchFromRegistry.ts
Expand Up @@ -48,6 +48,7 @@ export function createFetchFromRegistry (
let redirects = 0
let urlObject = new URL(url)
const originalHost = urlObject.host
/* eslint-disable no-await-in-loop */
while (true) {
const agentOptions = {
...defaultOpts,
Expand Down Expand Up @@ -77,6 +78,7 @@ export function createFetchFromRegistry (
if (!headers['authorization'] || originalHost === urlObject.host) continue
delete headers.authorization
}
/* eslint-enable no-await-in-loop */
}
}

Expand Down
4 changes: 2 additions & 2 deletions patching/plugin-commands-patching/src/patchRemove.ts
Expand Up @@ -57,13 +57,13 @@ export async function handler (opts: PatchRemoveCommandOptions, params: string[]
throw new PnpmError('NO_PATCHES_TO_REMOVE', 'There are no patches that need to be removed')
}

for (const patch of patchesToRemove) {
await Promise.all(patchesToRemove.map(async (patch) => {
if (Object.prototype.hasOwnProperty.call(patchedDependencies, patch)) {
const patchFile = path.join(lockfileDir, patchedDependencies[patch])
await fs.rm(patchFile, { force: true })
delete rootProjectManifest.pnpm!.patchedDependencies![patch]
}
}
}))

await writeProjectManifest(rootProjectManifest)

Expand Down
80 changes: 43 additions & 37 deletions pkg-manager/core/src/install/allProjectsAreUpToDate.ts
Expand Up @@ -74,44 +74,50 @@ async function linkedPackagesAreUpToDate (
snapshot: ProjectSnapshot
}
) {
for (const depField of DEPENDENCIES_FIELDS) {
const lockfileDeps = project.snapshot[depField]
const manifestDeps = project.manifest[depField]
if ((lockfileDeps == null) || (manifestDeps == null)) continue
const depNames = Object.keys(lockfileDeps)
for (const depName of depNames) {
const currentSpec = manifestDeps[depName]
if (!currentSpec) continue
const lockfileRef = lockfileDeps[depName]
const isLinked = lockfileRef.startsWith('link:')
if (
isLinked &&
(
currentSpec.startsWith('link:') ||
currentSpec.startsWith('file:') ||
currentSpec.startsWith('workspace:.')
)
) {
continue
}
const linkedDir = isLinked
? path.join(project.dir, lockfileRef.slice(5))
: workspacePackages?.[depName]?.[lockfileRef]?.dir
if (!linkedDir) continue
if (!linkWorkspacePackages && !currentSpec.startsWith('workspace:')) {
// we found a linked dir, but we don't want to use it, because it's not specified as a
// workspace:x.x.x dependency
continue
}
const linkedPkg = manifestsByDir[linkedDir] ?? await safeReadPackageJsonFromDir(linkedDir)
const availableRange = getVersionRange(currentSpec)
// This should pass the same options to semver as @pnpm/npm-resolver
const localPackageSatisfiesRange = availableRange === '*' || availableRange === '^' || availableRange === '~' ||
linkedPkg && semver.satisfies(linkedPkg.version, availableRange, { loose: true })
if (isLinked !== localPackageSatisfiesRange) return false
return pEvery(
DEPENDENCIES_FIELDS,
(depField) => {
const lockfileDeps = project.snapshot[depField]
const manifestDeps = project.manifest[depField]
if ((lockfileDeps == null) || (manifestDeps == null)) return true
const depNames = Object.keys(lockfileDeps)
return pEvery(
depNames,
async (depName) => {
const currentSpec = manifestDeps[depName]
if (!currentSpec) return true
const lockfileRef = lockfileDeps[depName]
const isLinked = lockfileRef.startsWith('link:')
if (
isLinked &&
(
currentSpec.startsWith('link:') ||
currentSpec.startsWith('file:') ||
currentSpec.startsWith('workspace:.')
)
) {
return true
}
const linkedDir = isLinked
? path.join(project.dir, lockfileRef.slice(5))
: workspacePackages?.[depName]?.[lockfileRef]?.dir
if (!linkedDir) return true
if (!linkWorkspacePackages && !currentSpec.startsWith('workspace:')) {
// we found a linked dir, but we don't want to use it, because it's not specified as a
// workspace:x.x.x dependency
return true
}
const linkedPkg = manifestsByDir[linkedDir] ?? await safeReadPackageJsonFromDir(linkedDir)
const availableRange = getVersionRange(currentSpec)
// This should pass the same options to semver as @pnpm/npm-resolver
const localPackageSatisfiesRange = availableRange === '*' || availableRange === '^' || availableRange === '~' ||
linkedPkg && semver.satisfies(linkedPkg.version, availableRange, { loose: true })
if (isLinked !== localPackageSatisfiesRange) return false
return true
}
)
}
}
return true
)
}

function getVersionRange (spec: string) {
Expand Down
2 changes: 2 additions & 0 deletions pkg-manager/core/src/install/index.ts
Expand Up @@ -513,6 +513,7 @@ Note that in CI environments, this setting is enabled by default.`,
let preferredSpecs: Record<string, string> | null = null

// TODO: make it concurrent
/* eslint-disable no-await-in-loop */
for (const project of projects) {
const projectOpts = {
...project,
Expand Down Expand Up @@ -608,6 +609,7 @@ Note that in CI environments, this setting is enabled by default.`,
}
}
}
/* eslint-enable no-await-in-loop */

async function installCase (project: any) { // eslint-disable-line
const wantedDependencies = getWantedDependencies(project.manifest, {
Expand Down
83 changes: 42 additions & 41 deletions pkg-manager/core/src/link/index.ts
Expand Up @@ -58,48 +58,49 @@ export async function link (
}, true)

const importerId = getLockfileImporterId(ctx.lockfileDir, opts.dir)
const linkedPkgs: Array<{ path: string, manifest: DependencyManifest, alias: string }> = []
const specsToUpsert = [] as PackageSpecObject[]

for (const linkFrom of linkFromPkgs) {
let linkFromPath: string
let linkFromAlias: string | undefined
if (typeof linkFrom === 'string') {
linkFromPath = linkFrom
} else {
linkFromPath = linkFrom.path
linkFromAlias = linkFrom.alias
}
const { manifest } = await readProjectManifest(linkFromPath) as { manifest: DependencyManifest }
if (typeof linkFrom === 'string' && manifest.name === undefined) {
throw new PnpmError('INVALID_PACKAGE_NAME', `Package in ${linkFromPath} must have a name field to be linked`)
}

const targetDependencyType = getDependencyTypeFromManifest(opts.manifest, manifest.name) ?? opts.targetDependenciesField

specsToUpsert.push({
alias: manifest.name,
pref: getPref(manifest.name, manifest.name, manifest.version, {
pinnedVersion: opts.pinnedVersion,
}),
saveType: (targetDependencyType ?? (ctx.manifest && guessDependencyType(manifest.name, ctx.manifest))) as DependenciesField,
const linkedPkgs = await Promise.all(
linkFromPkgs.map(async (linkFrom) => {
let linkFromPath: string
let linkFromAlias: string | undefined
if (typeof linkFrom === 'string') {
linkFromPath = linkFrom
} else {
linkFromPath = linkFrom.path
linkFromAlias = linkFrom.alias
}
const { manifest } = await readProjectManifest(linkFromPath) as { manifest: DependencyManifest }
if (typeof linkFrom === 'string' && manifest.name === undefined) {
throw new PnpmError('INVALID_PACKAGE_NAME', `Package in ${linkFromPath} must have a name field to be linked`)
}

const targetDependencyType = getDependencyTypeFromManifest(opts.manifest, manifest.name) ?? opts.targetDependenciesField

specsToUpsert.push({
alias: manifest.name,
pref: getPref(manifest.name, manifest.name, manifest.version, {
pinnedVersion: opts.pinnedVersion,
}),
saveType: (targetDependencyType ?? (ctx.manifest && guessDependencyType(manifest.name, ctx.manifest))) as DependenciesField,
})

const packagePath = normalize(path.relative(opts.dir, linkFromPath))
const addLinkOpts = {
linkedPkgName: linkFromAlias ?? manifest.name,
manifest: ctx.manifest,
packagePath,
}
addLinkToLockfile(ctx.currentLockfile.importers[importerId], addLinkOpts)
addLinkToLockfile(ctx.wantedLockfile.importers[importerId], addLinkOpts)

return {
alias: linkFromAlias ?? manifest.name,
manifest,
path: linkFromPath,
}
})

const packagePath = normalize(path.relative(opts.dir, linkFromPath))
const addLinkOpts = {
linkedPkgName: linkFromAlias ?? manifest.name,
manifest: ctx.manifest,
packagePath,
}
addLinkToLockfile(ctx.currentLockfile.importers[importerId], addLinkOpts)
addLinkToLockfile(ctx.wantedLockfile.importers[importerId], addLinkOpts)

linkedPkgs.push({
alias: linkFromAlias ?? manifest.name,
manifest,
path: linkFromPath,
})
}
)

const updatedCurrentLockfile = pruneSharedLockfile(ctx.currentLockfile)

Expand All @@ -110,7 +111,7 @@ export async function link (

// Linking should happen after removing orphans
// Otherwise would've been removed
for (const { alias, manifest, path } of linkedPkgs) {
await Promise.all(linkedPkgs.map(async ({ alias, manifest, path }) => {
// TODO: cover with test that linking reports with correct dependency types
const stu = specsToUpsert.find((s) => s.alias === manifest.name)
const targetDependencyType = getDependencyTypeFromManifest(opts.manifest, manifest.name) ?? opts.targetDependenciesField
Expand All @@ -119,7 +120,7 @@ export async function link (
linkedPackage: manifest,
prefix: opts.dir,
})
}
}))

const linkToBin = maybeOpts?.linkToBin ?? path.join(destModules, '.bin')
await linkBinsOfPackages(linkedPkgs.map((p) => ({ manifest: p.manifest, location: p.path })), linkToBin, {
Expand Down