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(pkgs-graph): speed up createPkgGraph when directory specifiers are present #6317

Merged
merged 2 commits into from Mar 29, 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
5 changes: 5 additions & 0 deletions .changeset/twelve-gifts-rescue.md
@@ -0,0 +1,5 @@
---
"@pnpm/workspace.pkgs-graph": patch
---

Speed up createPkgGraph when directory specifiers are present
36 changes: 30 additions & 6 deletions workspace/pkgs-graph/src/index.ts
Expand Up @@ -36,12 +36,8 @@ export function createPkgGraph<T> (pkgs: Array<Package & T>, opts?: {
} {
const pkgMap = createPkgMap(pkgs)
const pkgMapValues = Object.values(pkgMap)
const pkgMapByManifestName: Record<string, Package[] | undefined> = {}
for (const pkg of pkgMapValues) {
if (pkg.manifest.name) {
(pkgMapByManifestName[pkg.manifest.name] ??= []).push(pkg)
}
}
let pkgMapByManifestName: Record<string, Package[] | undefined> | undefined
let pkgMapByDir: Record<string, Package | undefined> | undefined
const unmatched: Array<{ pkgName: string, range: string }> = []
const graph = mapValues((pkg) => ({
dependencies: createNode(pkg),
Expand Down Expand Up @@ -73,15 +69,25 @@ export function createPkgGraph<T> (pkgs: Array<Package & T>, opts?: {
}

if (spec.type === 'directory') {
pkgMapByDir ??= getPkgMapByDir(pkgMapValues)
const resolvedPath = path.resolve(pkg.dir, spec.fetchSpec)
const found = pkgMapByDir[resolvedPath]
if (found) {
return found.dir
}

// Slow path; only needed when there are case mismatches on case-insensitive filesystems.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory we could detect whether it is a case-insensitive fs. In a future optimization. I already use this lib for detecting case sensitive dirs: https://github.com/zkochan/packages/tree/main/dir-is-case-sensitive

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a good idea. Another related optimization would be to ensure that all paths are "resolved", that way all of the path checks can skip re-resolving and instead use plain comparison and path.join + path.normalize.

const matchedPkg = pkgMapValues.find(pkg => path.relative(pkg.dir, spec.fetchSpec) === '')
if (matchedPkg == null) {
return ''
}
pkgMapByDir[resolvedPath] = matchedPkg
return matchedPkg.dir
}

if (spec.type !== 'version' && spec.type !== 'range') return ''

pkgMapByManifestName ??= getPkgMapByManifestName(pkgMapValues)
const pkgs = pkgMapByManifestName[depName]
if (!pkgs || pkgs.length === 0) return ''
const versions = pkgs.filter(({ manifest }) => manifest.version)
Expand Down Expand Up @@ -120,3 +126,21 @@ function createPkgMap (pkgs: Package[]): Record<string, Package> {
}
return pkgMap
}

function getPkgMapByManifestName (pkgMapValues: Package[]) {
const pkgMapByManifestName: Record<string, Package[] | undefined> = {}
for (const pkg of pkgMapValues) {
if (pkg.manifest.name) {
(pkgMapByManifestName[pkg.manifest.name] ??= []).push(pkg)
}
}
return pkgMapByManifestName
}

function getPkgMapByDir (pkgMapValues: Package[]) {
const pkgMapByDir: Record<string, Package | undefined> = {}
for (const pkg of pkgMapValues) {
pkgMapByDir[path.resolve(pkg.dir)] = pkg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resolve is defensive; since we're comparing resolved paths above, I wanted to ensure everything went through this algorithm before comparison. It's possible that this is not needed, however, there doesn't seem to be any negative performance hit.

}
return pkgMapByDir
}