fix(core): optimize pnpm lockfile parsing with pre-built indexes#33750
fix(core): optimize pnpm lockfile parsing with pre-built indexes#33750leosvelperez merged 2 commits intonrwl:masterfrom
Conversation
PNPM Parser Optimizations: - Use Object.entries() for single-pass iteration in matchPropValue instead of separate Object.values() + Object.keys() calls - Pre-build packageName -> key index Map for hoisted dependencies lookup (O(n*m) -> O(n+m) where n=packages, m=hoisted deps) Impact: Faster project graph creation for pnpm monorepos, especially those with many packages and hoisted dependencies.
👷 Deploy request for nx-docs pending review.Visit the deploys page to approve it
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
leosvelperez
left a comment
There was a problem hiding this comment.
Thanks for contributing! Please make hoistedKeysByPackage a required parameter.
700a583 to
4156b4f
Compare
|
View your CI Pipeline Execution ↗ for commit 4156b4f
☁️ Nx Cloud last updated this comment at |
|
The hoisted calculation improvement was a good catch. On a test repo we measured 5x improvement on that code section. Sadly, the Here are the reasons why:
|
) ## Current Behavior When parsing pnpm lockfiles to build the project graph: ### matchPropValue function - Uses separate `Object.values()` + `Object.keys()` iterations - Two passes over the same data to find a value and get its key ### Hoisted Dependencies Lookup - For each package, calls `Object.keys(hoistedDeps).find(k => k.startsWith(...))` - O(n) search performed m times = O(n*m) total ## Expected Behavior ### Single-pass Iteration ``` BEFORE: matchPropValue ┌─────────────────────────────────────────────────────────────┐ │ const index = Object.values(record).findIndex(v === key) │ │ if (index > -1) │ │ return Object.keys(record)[index] ← Another iteration! │ │ │ │ = 2 iterations over the same data │ └─────────────────────────────────────────────────────────────┘ AFTER: Object.entries() single pass ┌─────────────────────────────────────────────────────────────┐ │ for (const [name, version] of Object.entries(record)) { │ │ if (version === key) return name ← Early exit │ │ } │ │ │ │ = 1 iteration with early exit on match │ └─────────────────────────────────────────────────────────────┘ ``` ### Pre-built Index for Hoisted Dependencies ``` BEFORE: O(n*m) Hoisted Lookup ┌─────────────────────────────────────────────────────────────┐ │ for each package (n packages): │ │ Object.keys(hoistedDeps).find(k => k.startsWith(...)) │ │ ← O(m) search where m = hoisted deps │ │ │ │ Total: O(n * m) │ │ │ │ Example: 500 packages × 200 hoisted deps │ │ = 100,000 string comparisons │ └─────────────────────────────────────────────────────────────┘ AFTER: Pre-built Index Map O(n+m) ┌─────────────────────────────────────────────────────────────┐ │ Build hoistedKeysByPackage Map once: O(m) │ │ Map<packageName, hoistedKey> │ │ │ │ for each package (n packages): │ │ hoistedKeysByPackage.get(packageName) O(1) │ │ │ │ Total: O(n + m) │ │ │ │ Example: 500 packages + 200 hoisted deps │ │ = 700 operations (vs 100,000) │ └─────────────────────────────────────────────────────────────┘ ``` ## Impact For large pnpm monorepos: | Metric | Before | After | Improvement | |--------|--------|-------|-------------| | matchPropValue iterations | 2 | 1 | 50% fewer iterations | | Hoisted lookup complexity | O(n×m) | O(n+m) | ~100x for large repos | | String comparisons | n×m | n+m | Dramatic reduction | ## Related Issue(s) Contributes to #32669, #32254 ## Merge Dependencies This PR has no dependencies and can be merged independently. **Must be merged BEFORE:** #33751 ---
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
When parsing pnpm lockfiles to build the project graph:
matchPropValue function
Object.values()+Object.keys()iterationsHoisted Dependencies Lookup
Object.keys(hoistedDeps).find(k => k.startsWith(...))Expected Behavior
Single-pass Iteration
Pre-built Index for Hoisted Dependencies
Impact
For large pnpm monorepos:
Related Issue(s)
Contributes to #32669, #32254
Merge Dependencies
This PR has no dependencies and can be merged independently.
Must be merged BEFORE: #33751