Scala.js: Isolate linker in a separate classloader #629

keynmol · 2022-02-20T15:36:33Z

Remove Scala.js compiler dependency from mdoc
Remove direct linker/linker-interface dependency
Isolate linker in a separate class loader - this should address issues with conflicting IR versions on the classpath
- This introduces two new modules: jsWorkerApi (Java) and jsWorker (cross Scala)
Auto-detect Scala.js version from the SBT plugin
Mild cleanup of interactions with SJS linker inside the modifier
Remove usage of deprecated linker APIs
Updates Scala.js to 1.9.0

mdoc-js/src/main/scala/mdoc/modifiers/ScalajsWorker.scala

keynmol · 2022-02-20T16:07:04Z

As a motivating example, here's the class paths:

 Compile classpath: List(
    file:////home/runner/work/mdoc/mdoc/tests/jsdocs/target/scala-3.1.1/classes, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala3-library_sjs1_3/3.1.1/scala3-library_sjs1_3-3.1.1.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-library_2.13/1.9.0/scalajs-library_2.13-1.9.0.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-dom_sjs1_3/2.0.0/scalajs-dom_sjs1_3-2.0.0.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.8/scala-library-2.13.8.jar)

 Linker classpath: List(
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-linker_2.13/1.9.0/scalajs-linker_2.13-1.9.0.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.8/scala-library-2.13.8.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-linker-interface_2.13/1.9.0/scalajs-linker-interface_2.13-1.9.0.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-ir_2.13/1.9.0/scalajs-ir_2.13-1.9.0.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/com/google/javascript/closure-compiler/v20220202/closure-compiler-v20220202.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/modules/scala-parallel-collections_2.13/0.2.0/scala-parallel-collections_2.13-0.2.0.jar, 
    file:////home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-logging_2.13/1.1.1/scalajs-logging_2.13-1.1.1.jar)

The "compiler class path" is passed to the compiler
The linker class path is used to instantiate the ClassLoader in which linker is isolated
The compiler class path is analysed by PathIRContainer

mdoc-js/src/main/scala/mdoc/modifiers/ScalajsWorker.scala

…build

armanbilge · 2022-02-21T13:04:48Z

build.sbt

+// This will work as long as mdoc has scala-js SBT plugin
+def scalajs = {
+  val klass =
+    Class.forName("org.scalajs.ir.ScalaJSVersions", true, getClass.getClassLoader())
+  val method = klass.getMethod("current")
+  method.invoke(null).asInstanceOf[String]
+}


Is there any concern of sbt loading different plugins in different classloaders? I may have encountered this but never confirmed it.

armanbilge · 2022-02-21T21:21:00Z

🎉 😍

mdoc-js/src/main/scala/mdoc/modifiers/ScalajsWorker.scala

lolgab · 2022-02-22T11:35:57Z

Since the ScalaJS toolchain is available for the same Scala version as the one you're using, I think you can write the API module in Scala if you want. We use Java in Mill only when the Worker implementation uses a different Scala binary version than the Plugin. In your case everything is on Scala 2.12

keynmol · 2022-02-22T12:14:15Z

mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala

+  lazy val mdocJSWorkerClasspath = taskKey[Option[Seq[File]]](
+    "Optional classpath to use for Mdoc.js worker - " +
+      "if not provided, the classpath will be formed by resolving the worker dependency"
+  )
+


We need this separate task (not part of autoImport) to make mdoc's own tests to work - we can't introduce a dependency on mdoc-js-worker, and this would be reliable than placing dependsOn(jsWorker / publishLocal) in all the possible situations where this module must be available.

keynmol · 2022-02-22T12:19:04Z

mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala

+          MdocJSConfiguration(
+            scalacOptions = options.options,
+            compileClasspath = options.classpath,
+            linkerClassPath = getJars(linkerDependency) ++ workerClasspath,
+            moduleKind = options.moduleKind,
+            jsLibraries = libraries
+          ).writeTo(props)


This is a strictly unnecessary refactor, but it's a small step towards my goal of weaning off mdoc's build of loading MdocPlugin at build time.

Main issue is that it forces 2.12 on all the modules that are used to build the site
It also causes some weird issues when compiling the build, i.e. I currently can't resolve the deprecation warnings because the build just fails to compile when ++2.12 is used

sbt on 2.13 would be so nice.

keynmol · 2022-02-22T12:20:03Z

mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala

-        props.put(
-          s"js-libraries",
-          mdocJSLibraries.value.map(_.data).mkString(File.pathSeparator)
-        )


If I were to guess, this setting was extracted from the block above because SBT disallows invoking the task in anonymous lambda, and not because it's a valid usecase to not have mdoc.js configuration but still use js-libraries?

keynmol · 2022-02-22T12:20:24Z

project/plugins.sbt

@@ -1,7 +1,7 @@
 addSbtPlugin("com.eed3si9n" % "sbt-buildinfo" % "0.10.0")
 addSbtPlugin("com.eed3si9n" % "sbt-assembly" % "1.1.0")
 addSbtPlugin("com.github.sbt" % "sbt-ci-release" % "1.5.10")
-addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.7.1")
+addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.9.0")


Upgrade Mdoc's own Scala.js to 1.9.0

keynmol · 2022-02-22T12:25:05Z

build.sbt

-    mdocJS := Some(jsdocs)
+    mdocJS := Some(jsdocs),
+    MdocPlugin.mdocJSWorkerClasspath := {
+      val _ = (jsWorker / Compile / compile).value


Dependency on classDirectory is not enough to trigger compilation.
The user's build will not include this setting (mdocJSWorkerClasspath) at all, it's here only because mdoc bootstraps itself

keynmol · 2022-02-22T12:47:11Z

mdoc-sbt/src/sbt-test/sbt-mdoc/basic/project/plugins.sbt

@@ -1,2 +1,2 @@
-addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.7.0")
+addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.9.0")


This version is intended to be updated with each new release of Scala.js

keynmol · 2022-02-22T12:48:01Z

mdoc-sbt/src/sbt-test/sbt-mdoc/scalajs-1.7/project/plugins.sbt

@@ -0,0 +1,2 @@
+addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.7.0")


Frozen tests at this Scala.js version as it's the last one that was working in the old setup of the plugin, so we want to keep it here to make sure future changes won't break older versions.

tgodzik

Looks good! Just some questions from me, but nothing blocking.

tgodzik · 2022-02-22T17:52:06Z

mdoc-js-worker/src/main/scala/ScalaJSWorker.scala

+        .fromClasspath(x.toSeq)
+        .map(_._1)
+        .flatMap(cache.cached),
+      Duration.Inf


Will this not cause any issues to use Duration.Inf? Maybe we should use CompletableFuture as the result value instead?

To be honest the entire JsModifier process is very synchronous and futures are always waited on, even in the current main branch.

We can always change the interface as it is internal, so I wanted to keep it simple and avoid introducing potentially configurable timeouts because its brittle.

tgodzik · 2022-02-22T17:54:20Z

mdoc-js/src/main/scala/mdoc/modifiers/JsModifier.scala

-      val linking =
-        linker.link(virtualIrFiles ++ sjsir, Nil, LinkerOutput.apply(output), sjsLogger)
-      Await.result(linking, Duration.Inf)
+      val linkingReport = scalajsApi.get.link(sjsirFiles.toArray)


Should we match on the Option instead? Or maybe throw a better exception if scalajsApi is empty?

olafurpg

LGTM 👍 Just a few minor comments, feel free to ignore. Thank you for looking into this! It's hairy business to fiddle with classloaders, but it's really wonderful IMO that the JVM makes it possible at all

olafurpg · 2022-02-23T12:21:46Z

build.sbt

@@ -395,22 +414,30 @@ lazy val plugin = project
  )
  .enablePlugins(ScriptedPlugin)

+lazy val jsApi =
+  project
+    .in(file("mdoc-js-api"))


Can we name this mdoc-js-interfaces for consistency with mtags-interfaces? (and other artifacts using the -interfaces suffix)

olafurpg · 2022-02-23T12:23:58Z

mdoc-js/src/main/scala/mdoc/modifiers/JsModifier.scala

+            ScalaJSClassloader.create(linkerClasspath.entries.map(_.toURI.toURL()).toArray)
+          scalajsApi = Some(
+            loader
+              .loadClass("mdoc.js.worker.ScalaJSWorker")


Can we use service loaders here instead of manual reflection? Service loaders have several tooling benefits, for example they are automatically understood by native-image (although that's not relevant here). https://docs.oracle.com/javase/8/docs/api/java/util/ServiceLoader.html

olafurpg · 2022-02-23T12:25:30Z

mdoc-js/src/main/scala/mdoc/modifiers/ScalajsClassloader.scala

+import java.net.URLClassLoader
+
+final class FilteringClassLoader(parent: ClassLoader) extends ClassLoader(parent) {
+  private val parentPrefixes = List(


nit: the loadClass is a fairly hot path so I'd try to avoid List here

Suggested change

private val parentPrefixes = List(

private val parentPrefixes = Array(

olafurpg · 2022-02-23T12:27:28Z

mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala

+          MdocJSConfiguration(
+            scalacOptions = options.options,
+            compileClasspath = options.classpath,
+            linkerClassPath = getJars(linkerDependency) ++ workerClasspath,
+            moduleKind = options.moduleKind,
+            jsLibraries = libraries
+          ).writeTo(props)


sbt on 2.13 would be so nice.

olafurpg · 2022-02-23T15:47:23Z

mdoc-js-worker/src/main/scala/ScalaJSWorkerProvider.scala

+      case Level.Error => Error
+    }
+  }
+  def create(config: i.ScalajsConfig, logger: i.ScalajsLogger) = {


nit: missing result type

olafurpg · 2022-02-23T15:47:45Z

mdoc-js/src/main/scala/mdoc/modifiers/JsModifier.scala

-              .newInstance(scalajsConfig(new ScalajsConfig, config), sjsLogger)
-              .asInstanceOf[ScalajsWorkerApi]
+            ServiceLoader
+              .load(classOf[ScalajsWorkerProvider], loader)


Do horrible things with classloaders

8f89013

keynmol force-pushed the scalajs-classloaders branch from 5471c47 to 8f89013 Compare February 20, 2022 15:37

keynmol commented Feb 20, 2022

View reviewed changes

mdoc-js/src/main/scala/mdoc/modifiers/ScalajsWorker.scala Outdated Show resolved Hide resolved

keynmol commented Feb 20, 2022

View reviewed changes

mdoc-js/src/main/scala/mdoc/modifiers/ScalajsWorker.scala Outdated Show resolved Hide resolved

keynmol added 3 commits February 20, 2022 15:42

Use 3.1.1

99cda5b

Fix 2.12

be09cc1

No idea why these conflict

1ff65a1

sjrd reviewed Feb 21, 2022

View reviewed changes

mdoc-js/src/main/scala/mdoc/modifiers/ScalajsWorker.scala Outdated Show resolved Hide resolved

Clean up and return incremental linker

28f6100

keynmol changed the title ~~Scala.js: Do horrible things with classloaders~~ Scala.js: Isolate linker in a separate classloader Feb 21, 2022

keynmol added 2 commits February 21, 2022 12:29

Remove Scala.js compiler dependency and auto-detect Scala.js version

03ffd3f

Revert moduleKind settings, auto-detect Scala.js version in the main …

fff4fcb

…build

armanbilge reviewed Feb 21, 2022

View reviewed changes

keynmol mentioned this pull request Feb 21, 2022

mdoc.js is brittle to Scala.js updates #601

Closed

keynmol added 5 commits February 21, 2022 14:09

Fix IR checker loading in wrong classloader

9a4593f

Make Scala 3 a special case

c87183d

Use only reflection

891c595

Reimplement StandardConfig

cefa731

Hopefully fix Windows

13db99b

keynmol commented Feb 21, 2022

View reviewed changes

mdoc-js/src/main/scala/mdoc/modifiers/ScalajsWorker.scala Outdated Show resolved Hide resolved

keynmol added 2 commits February 22, 2022 11:22

Replace reflection with a Java interface

e89e05f

Oops

4c03e3c

keynmol added 2 commits February 22, 2022 11:58

Clean up interfaces and correctly set task dependencies

e9649f4

Fix config not being modified

2923285

keynmol commented Feb 22, 2022

View reviewed changes

Set semantics correctly

a85e032

keynmol commented Feb 22, 2022

View reviewed changes

keynmol added 2 commits February 22, 2022 12:35

Ensure Java interfaces produce JVM8 bytecode

55a0df7

Add scripted test for older Scala.js version

bea32be

keynmol commented Feb 22, 2022

View reviewed changes

keynmol requested review from tgodzik and olafurpg February 22, 2022 16:11

Fix and update test-release script

b5c81a5

tgodzik approved these changes Feb 22, 2022

View reviewed changes

This was referenced Feb 22, 2022

Update scala3-compiler, scala3-library, ... to 3.1.1 #624

Closed

Update Scala.js to 1.8.0 #597

Closed

olafurpg reviewed Feb 23, 2022

View reviewed changes

olafurpg approved these changes Feb 23, 2022

View reviewed changes

keynmol added 3 commits February 23, 2022 15:31

api -> interfaces, use ServiceLoader instead of reflection

e8b73ef

Remove dependency on Scalajs logging

878465d

Fix test-release.sh script

7582071

olafurpg reviewed Feb 23, 2022

View reviewed changes

Explicit return type

15393f1

keynmol merged commit 9014e2d into scalameta:main Feb 24, 2022

keynmol deleted the scalajs-classloaders branch February 24, 2022 09:52

armanbilge mentioned this pull request Feb 24, 2022

Release mdoc against Scala.js 1.8.x #590

Closed

keynmol mentioned this pull request Feb 27, 2022

Release mdoc against Scala.js 1.9.x #617

Closed

lolgab mentioned this pull request Nov 25, 2022

Make JsEnv extensible com-lihaoyi/mill#2144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala.js: Isolate linker in a separate classloader #629

Scala.js: Isolate linker in a separate classloader #629

keynmol commented Feb 20, 2022 •

edited

Loading

keynmol commented Feb 20, 2022

armanbilge Feb 21, 2022

armanbilge commented Feb 21, 2022

lolgab commented Feb 22, 2022 •

edited

Loading

keynmol Feb 22, 2022

keynmol Feb 22, 2022

olafurpg Feb 23, 2022

keynmol Feb 22, 2022

keynmol Feb 22, 2022

keynmol Feb 22, 2022

keynmol Feb 22, 2022

keynmol Feb 22, 2022

tgodzik left a comment

tgodzik Feb 22, 2022

keynmol Feb 22, 2022

tgodzik Feb 23, 2022

tgodzik Feb 22, 2022

olafurpg left a comment

olafurpg Feb 23, 2022

olafurpg Feb 23, 2022

olafurpg Feb 23, 2022

olafurpg Feb 23, 2022

olafurpg Feb 23, 2022

olafurpg Feb 23, 2022

		@@ -1,2 +1,2 @@
		addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.7.0")
		addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.9.0")

		@@ -0,0 +1,2 @@
		addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.7.0")

	private val parentPrefixes = List(
	private val parentPrefixes = Array(

Scala.js: Isolate linker in a separate classloader #629

Scala.js: Isolate linker in a separate classloader #629

Conversation

keynmol commented Feb 20, 2022 • edited Loading

keynmol commented Feb 20, 2022

Choose a reason for hiding this comment

armanbilge commented Feb 21, 2022

lolgab commented Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keynmol commented Feb 20, 2022 •

edited

Loading

lolgab commented Feb 22, 2022 •

edited

Loading