fix(core): improve buildExplicitTypeScriptDependnecies performance #33963
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
View your CI Pipeline Execution ↗ for commit 6ef7b46
☁️ Nx Cloud last updated this comment at |
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts
Show resolved
Hide resolved
|
|
||
| const { findImports } = require('../../../../native'); | ||
|
|
||
| // TODO(jon): check why this function is very slow and whether we it can be optimized |
There was a problem hiding this comment.
All files get processed in parallel... it goes as fast as the largest file which is the slowest to process. IF this is taking long it likely mans that there is a considerably large typescript file. You can see which one it is with NATIVE_FILE_LOGGING tbh, I don't think the TODO comment is necessary.
|
|
||
| const { findImports } = require('../../../../native'); | ||
|
|
||
| // TODO(jon): check why this function is very slow and whether we it can be optimized |
There was a problem hiding this comment.
All files get processed in parallel... it goes as fast as the largest file which is the slowest to process. IF this is taking long it likely mans that there is a considerably large typescript file. You can see which one it is with NATIVE_FILE_LOGGING tbh, I don't think the TODO comment is necessary.
| for (const [project, fileData] of Object.entries( | ||
| ctx.filesToProcess.projectFileMap | ||
| )) { | ||
| Object.keys(ctx.filesToProcess.projectFileMap).forEach((project) => { |
There was a problem hiding this comment.
Does this actually yield a performance benefit?
There was a problem hiding this comment.
Yes, I don't have the numbers anymore, but it shaved off 100+ms IIRC.
| dynamicImportExpressions, | ||
| } of imports) { | ||
| for (let i = 0; i < imports.length; i++) { | ||
| const { |
There was a problem hiding this comment.
Does this actually yield any performance benefit?
There was a problem hiding this comment.
It does, however, in this location it's not large impact so if you don't like this old-school syntax we can revert this block.
| if (this.packageJsonResolutionCache.has(packageJsonPath)) { | ||
| return this.packageJsonResolutionCache.get(packageJsonPath); | ||
| } | ||
| if (existsSync(packageJsonPath)) { |
There was a problem hiding this comment.
we should get rid of this existsSync.. it will throw an error if it doesn't exist which is already handled
There was a problem hiding this comment.
It's more common for the file to not exist (falling through down to the root package.json). The existSync saves time as readFileSync + try/catch is noticeably slower at scale.
| // Package.json doesn't exist, keep traversing | ||
| } | ||
| if (dir === workspaceRoot) { | ||
| return null; |
There was a problem hiding this comment.
So before.. we were able to.. resolve outside of the workspace root?
Have we considered if this actually helps when the workspaceRoot is not the same as the node_modules root? WHat if someone has like..
repo-root/package.json
repo-root/node_modules
repo-root/nx-workspace-root/nx.json
If this doesn't yield a major perf behefit, I would not make htis change for now
There was a problem hiding this comment.
I can test on the customer repo if reverting this change impacts performance.
I was more worried about correctness. Omitting the version is root package.json would cause traversal down to machine root, hoping no other package.json is found it the way. Also, this would be repeated for every single package name lookup.
| > = {}, | ||
| private readonly npmResolutionCache: NpmResolutionCache = defaultNpmResolutionCache | ||
| private readonly npmResolutionCache: NpmResolutionCache = defaultNpmResolutionCache, | ||
| private readonly packageJsonResolutionCache: PackageJsonResolutionCache = defaultPackageJsonResolutionCache |
There was a problem hiding this comment.
I did not review too carefully.. but just to sanity check me...
This is OK for the daemon because everytime we calculate dependencies, we create a brand new project locator with no cache right? So this cache will not remain between different graph calculations?
There was a problem hiding this comment.
Yes, exactly. The cache is reused only during the single CLI command/daemon OP lifetime.
On test repo reduces the
nx/js/dependencies-and-lockfile:createDependencies:10506ms3665msCurrent Behavior
Expected Behavior
Related Issue(s)
Fixes #