-
-
Notifications
You must be signed in to change notification settings - Fork 421
Limit parallel Scala.js linking jobs to avoid high memory pressure #6260
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
base: main
Are you sure you want to change the base?
Conversation
Fix: com-lihaoyi#6226 I just hardcoded the limit to `2`.
72beb07 to
d2aa908
Compare
davesmith00000
left a comment
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.
LGTM, thanks for picking this up @lefou. 🙏
It's kind of unsatisfying having to hardcore a value here, but I believe that setting a hardcoded sensible value is a better situation than people accidentally getting into an OutOfMemory blackhole by performing a common action, such as running all their tests.
| def scalaJSWorker: Worker[ScalaJSWorker] = Task.Worker { | ||
| new ScalaJSWorker( | ||
| jobs = Task.ctx().jobs, | ||
| linkerJobs = 2 |
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.
Just a suggestion: Perhaps this value could be exposed on ScalaJSModuleAPI? It can have a nice low default but allow people to tweak it to their needs or based on some environmental heuristic? E.g. They have a massive CI server and can afford to open up the parallelism.
Exposing it on the API also slightly improves the transparency around what's going on here, but perhaps this will need to be documented somehow?
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.
Yeah, I already thought about how to configure it, but didn't want to overengineer it.
The natural place for a config task would be the ScalaJSWorker, which is currently not designed to be customized, in a way other worker are, for example the JvmWorkerModule. Also, since there are potentially more than one ScalaJSWorker, we would need to introduce a new shared worker, so this route isn't a trivial change.
What would be somewhat easier is accepting an environment variable.
Also, we should converge to a "sensible default". I don't work with Scala.JS often, so I have no "feeling" for what a good value might be. We might also apply some logic based on heuristics, which I don't have.
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.
I'm not sure what reasonable heuristics you could sensibly apply, and I suspect attempting to do that might be a lot of work for not a lot of reward. 🤷
FWIW, @lolgab was suggesting a concurrency of 1 in a discussion on Discord, and I'm using 2 in CI:
https://github.com/PurpleKingdomGames/indigoengine/blob/main/ci.sh#L9-L10
Fix: #6226
I just hardcoded the limit to
2for now.