-
Notifications
You must be signed in to change notification settings - Fork 135
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
fix: add concurrency limit to fs calls #301
Conversation
Codecov Report
@@ Coverage Diff @@
## main #301 +/- ##
==========================================
+ Coverage 80.45% 80.57% +0.11%
==========================================
Files 13 13
Lines 1489 1498 +9
Branches 555 556 +1
==========================================
+ Hits 1198 1207 +9
Misses 121 121
Partials 170 170
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
62c49bf
to
f098d71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a PR description. Also can you add a test to prove this fixes it? (presumably this is fixing something due to the title of the PR?)
f098d71
to
3e1d2b2
Compare
3e1d2b2
to
90d78fb
Compare
src/node-file-trace.ts
Outdated
try { | ||
const link = await fsReadlink(path); | ||
await this.fileIOQueue.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't async operations.
await this.fileIOQueue.release(); | |
this.fileIOQueue.release(); |
src/node-file-trace.ts
Outdated
@@ -162,6 +170,7 @@ export class Job { | |||
return link; | |||
} | |||
catch (e) { | |||
await this.fileIOQueue.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await this.fileIOQueue.release(); | |
this.fileIOQueue.release(); |
src/node-file-trace.ts
Outdated
try { | ||
const stats = await fsStat(path); | ||
await this.fileIOQueue.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await this.fileIOQueue.release(); | |
this.fileIOQueue.release(); |
src/node-file-trace.ts
Outdated
this.statCache.set(path, stats); | ||
return stats; | ||
} | ||
catch (e) { | ||
await this.fileIOQueue.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await this.fileIOQueue.release(); | |
this.fileIOQueue.release(); |
src/node-file-trace.ts
Outdated
try { | ||
const source = (await fsReadFile(path)).toString(); | ||
this.fileCache.set(path, source); | ||
await this.fileIOQueue.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await this.fileIOQueue.release(); | |
this.fileIOQueue.release(); |
src/node-file-trace.ts
Outdated
return source; | ||
} | ||
catch (e) { | ||
await this.fileIOQueue.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await this.fileIOQueue.release(); | |
this.fileIOQueue.release(); |
src/node-file-trace.ts
Outdated
this.fileCache.set(path, source); | ||
await this.fileIOQueue.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Flip these to make it consistent with the others:
this.fileCache.set(path, source); | |
await this.fileIOQueue.release(); | |
this.fileIOQueue.release(); | |
this.fileCache.set(path, source); |
src/node-file-trace.ts
Outdated
@@ -79,6 +81,9 @@ export class Job { | |||
ts = true, | |||
analysis = {}, | |||
cache, | |||
// we use a default of 1024 concurrency to balance | |||
// performance and memory usage for fs operations | |||
fileIOConcurrency = Number(process.env.FILE_IO_CONCURRENCY) || 1024, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let not use an env var. Instead, the consumer should pass the fileIOConcurrency
option to nodeFileTrace()
We also need to update the README.md with the new option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
### Related Issues Updates to the latest version of `@vercel/nft` which adds fs concurrency limits to help alleviate memory usage. x-ref: vercel/nft#301 ### 📋 Checklist <!-- Please keep your PR as a Draft until the checklist is complete --> #### Tests - [ ] The code changed/added as part of this PR has been covered with tests - [ ] All tests pass locally with `yarn test-unit` #### Code Review - [ ] This PR has a concise title and thorough description useful to a reviewer - [ ] Issue from task tracker has a link to this PR
This adds concurrency limits to our
fs
calls to attempt to alleviate memory issues with too many promises being stored in memory at once.x-ref: slack thread