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

Read carefully #1696

Closed
wants to merge 2 commits into from
Closed

Read carefully #1696

wants to merge 2 commits into from

Conversation

kriskowal
Copy link
Member

Fixes #1593, the bundler and compartment-mapper read many files to build a bundle and these reads are by default aggressively concurrent. This can lead to the exhaustion of file descriptors. This is generally silly because a file system does not tend to improve throughput with any amount of concurrency.

However, the compartment mapper is sufficiently general to run on any read powers, so it would be poor form to use a synchronous read function.

With this change, the compartment mapper classifies read errors in Node.js:

  • those that indicate that a read was interrupted, as can occur if reading when the process receives a signal
  • those that indicate that a read failed because file descriptors were exhausted
  • those that indicate that a read failed because a file did not exist, or an entry existed but was not a file

…and monitors the throughput of reads. The governor implements an hybrid control loop based on AIMD (additive increase, multiplicative decrease) and CoDel (control delay).

  • if throughput increases, we increase the concurrency limit by 1
  • if throughput decreases, we apply a proportional decrease to the concurrency limit
  • if we exhaust file descriptors, we halve the concurrency limit
  • if we are interrupted, we try again and do not apply the delay against the throughput

And we also incidentally are more careful to distinguish these errors from missing entries when searching for a file (maybeRead).

A worthy critique of this PR is: perhaps instead of this level of sophistication you could just limit the Node.js read powers to a single concurrent read.

@kriskowal kriskowal force-pushed the kriskowal/1593-read-carefully branch from f92d306 to 7021787 Compare July 25, 2023 22:17
@kriskowal kriskowal requested a review from michaelfig July 25, 2023 22:32
@kriskowal kriskowal mentioned this pull request Jul 25, 2023
@kriskowal kriskowal force-pushed the kriskowal/1593-read-carefully branch from 7021787 to ed013b9 Compare July 25, 2023 23:37
@kriskowal
Copy link
Member Author

Closing in favor of #1697.

@kriskowal kriskowal closed this Jul 26, 2023
@kriskowal kriskowal deleted the kriskowal/1593-read-carefully branch July 26, 2023 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundle tools should handle EMFILE gracefully
1 participant