Skip to content

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.

Sign up for GitHub

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

Open
lefou wants to merge 7 commits into
base: main
Choose a base branch
from
+77 −5
Open

Limit parallel Scala.js linking jobs to avoid high memory pressure #6260

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e85906b
Limit parallel Scala.js linking jobs to avoid high memory pressure
lefou Nov 27, 2025
d2aa908
Clenaup
lefou Nov 27, 2025
8fe304b
typo
lefou Nov 27, 2025
5e8d01b
Clear the linker state after each run
lefou Dec 5, 2025
fa300b4
Revert "Clear the linker state after each run"
lefou Dec 5, 2025
ae3fd39
Merge branch 'main' into tr-scalajs-linker-parallel
lefou Dec 5, 2025
20fbd87
Merge branch 'main' into tr-scalajs-linker-parallel
lefou Dec 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions libs/scalajslib/src/mill/scalajslib/worker/ParallelismLimiter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package mill.scalajslib.worker

import java.util.concurrent.Semaphore

/**
* Limit the parallelism of jobs run via [[runLimited]].
* @param maxJobs The maximal parallelism
*/
class ParallelismLimiter(maxJobs: Int) {

private val linkerJobsSemaphore = Semaphore(maxJobs)

def runLimited[T](thunk: => T): T = {
linkerJobsSemaphore.acquire()
try {
thunk
} finally {
linkerJobsSemaphore.release()
}
}

}
16 changes: 11 additions & 5 deletions libs/scalajslib/src/mill/scalajslib/worker/ScalaJSWorker.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package mill.scalajslib.worker

import mill.*
import mill.scalajslib.api
import mill.scalajslib.worker.{api => workerApi}
import mill.scalajslib.worker.api as workerApi
import mill.api.TaskCtx
import mill.api.Result
import mill.api.daemon.internal.internal
Expand All @@ -13,7 +13,7 @@ import java.io.File
import java.net.URLClassLoader

@internal
private[scalajslib] class ScalaJSWorker(jobs: Int)
private[scalajslib] class ScalaJSWorker(jobs: Int, linkerJobs: Int)
extends CachedFactory[Seq[mill.PathRef], (URLClassLoader, workerApi.ScalaJSWorkerApi)] {

override def setup(key: Seq[PathRef]) = {
Expand Down Expand Up @@ -190,6 +190,8 @@ private[scalajslib] class ScalaJSWorker(jobs: Int)
}
}

private val linkerJobLimiter = ParallelismLimiter(linkerJobs)
Copy link
Member

@lolgab lolgab Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should pass linkerJobs instead of jobs to

    val bridge = cl
      .loadClass("mill.scalajslib.worker.ScalaJSWorkerImpl")
      .getDeclaredConstructor(classOf[Int])
      .newInstance(jobs)
      .asInstanceOf[workerApi.ScalaJSWorkerApi]

Since we are running two linker jobs at a time to save memory, if we store 8 different ones in memory, we aren't saving as much memory as we want.

Copy link
Member Author

@lefou lefou Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My working hypothesis was, that the high memory usage is required while the linking is in process, but most of it gets freed afterwards. That means, by delaying/synchronizing linking jobs, we already reduce the memory pressure. #6260 (comment) seems to support or at least not counter support this hypothesis.

Copy link
Member

@lolgab lolgab Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider that the test was performed with the code to clear the linker after every link step, which makes sense to clean the linker memory afterwards. If we keep the linkers in memory and do not clear them, the test could give different results.


def link(
toolsClasspath: Seq[mill.PathRef],
runClasspath: Seq[mill.PathRef],
Expand All @@ -207,7 +209,7 @@ private[scalajslib] class ScalaJSWorker(jobs: Int)
minify: Boolean,
importMap: Seq[api.ESModuleImportMapping],
experimentalUseWebAssembly: Boolean
): Result[api.Report] = {
): Result[api.Report] = linkerJobLimiter.runLimited {
withValue(toolsClasspath) { case (_, bridge) =>
bridge.link(
runClasspath = runClasspath.iterator.map(_.path.toNIO).toSeq,
Expand Down Expand Up @@ -258,7 +260,11 @@ private[scalajslib] class ScalaJSWorker(jobs: Int)
@internal
private[scalajslib] object ScalaJSWorkerExternalModule extends mill.api.ExternalModule {

def scalaJSWorker: Worker[ScalaJSWorker] =
Task.Worker { new ScalaJSWorker(Task.ctx().jobs) }
def scalaJSWorker: Worker[ScalaJSWorker] = Task.Worker {
new ScalaJSWorker(
jobs = Task.ctx().jobs,
linkerJobs = 2
Copy link
Contributor

@davesmith00000 davesmith00000 Nov 28, 2025

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?

Copy link
Member Author

@lefou lefou Nov 28, 2025 •
edited
Loading

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.

Copy link
Contributor

@davesmith00000 davesmith00000 Nov 28, 2025

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

)
}
lazy val millDiscover = Discover[this.type]
}
44 changes: 44 additions & 0 deletions libs/scalajslib/test/src/mill/scalajslib/worker/ParallelismLimiterTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package mill.scalajslib.worker

import utest.*

import java.util.concurrent.atomic.AtomicInteger

class ParallelismLimiterTests extends TestSuite {

override def tests = Tests {
test("limitedJobs") {

val maxJobs = 3
val limiter = ParallelismLimiter(maxJobs)

val concurrentCount = new AtomicInteger(0)
val maxObserved = new AtomicInteger(0)

def work(i: Int, workTimeMs: Int): Unit = {
val before = concurrentCount.incrementAndGet()
maxObserved.updateAndGet(v => Math.max(v, before))

Thread.sleep(workTimeMs)

val after = concurrentCount.decrementAndGet()
assert(after >= 0)
}

val tasks = (1 to 10).map { i =>
new Thread(() =>
limiter.runLimited {
work(i, 50)
}
)
}

tasks.foreach(_.start())
tasks.foreach(_.join())

assert(maxObserved.get() <= maxJobs)
}

}

}
7 changes: 6 additions & 1 deletion libs/scalajslib/worker/1/src/mill/scalajslib/worker/ScalaJSWorkerImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,12 @@ class ScalaJSWorkerImpl(jobs: Int) extends ScalaJSWorkerApi {
Left(e.getMessage)
}

Await.result(resultFuture, Duration.Inf)
val res = Await.result(resultFuture, Duration.Inf)
linker match {
case cl: ClearableLinker => cl.clear()
case _ => // no-op
}
res
Copy link
Member

@lolgab lolgab Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks Scala.js incremental linking.

Copy link
Member Author

@lefou lefou Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert it. The API docs don't tell, that this is related to incremental linking.

Copy link
Member Author

@lefou lefou Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert it.

}

def run(config: JsEnvConfig, report: Report): Unit = {
Expand Down
Loading