From 88e77d4dcd9e510ea1f37e47cbadea3ae5b2dc2e Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Mon, 20 May 2024 18:03:43 +1200 Subject: [PATCH 01/16] Fix #4985: Create separate tagger for FewestModules --- .../FewestModulesAnalyzer.scala | 2 +- .../modulesplitter/FewestModulesTagger.scala | 313 ++++++++++++++++++ .../SmallModulesForAnalyzer.scala | 2 +- ...r.scala => SmallestModulesForTagger.scala} | 15 +- 4 files changed, 323 insertions(+), 9 deletions(-) create mode 100644 linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala rename linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/{Tagger.scala => SmallestModulesForTagger.scala} (95%) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesAnalyzer.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesAnalyzer.scala index 5b96f5eaf3..0a80a01286 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesAnalyzer.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesAnalyzer.scala @@ -33,7 +33,7 @@ private[modulesplitter] final class FewestModulesAnalyzer extends ModuleAnalyzer new SingleModuleAnalysis(info.publicModuleDependencies.head._1) } else { val modulesToAvoid = info.publicModuleDependencies.keys - val moduleMap = new Tagger(info).tagAll(modulesToAvoid) + val moduleMap = new FewestModulesTagger(info).tagAll(modulesToAvoid) new FullAnalysis(moduleMap) } diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala new file mode 100644 index 0000000000..b95a369f57 --- /dev/null +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala @@ -0,0 +1,313 @@ +/* + * Scala.js (https://www.scala-js.org/) + * + * Copyright EPFL. + * + * Licensed under Apache License 2.0 + * (https://www.apache.org/licenses/LICENSE-2.0). + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +package org.scalajs.linker.frontend.modulesplitter + +import org.scalajs.ir.Names.ClassName +import org.scalajs.ir.SHA1 +import org.scalajs.linker.frontend.modulesplitter.InternalModuleIDGenerator.ForDigests +import org.scalajs.linker.standard.ModuleSet.ModuleID + +import java.nio.ByteBuffer +import java.nio.charset.StandardCharsets +import scala.annotation.tailrec +import scala.collection.{immutable, mutable} + +/** FewestModulesTagger groups classes into coarse modules. + * + * It is the primary mechanism for the FewestModulesAnalyzer but also used + * by the SmallModulesForAnalyzer. + * + * To group classes into modules appropriately, we want to know for + * each class, "how" it can be reached. In practice, this means we + * record the path from the original public module and every + * dynamic import hop we made. + * + * Of all these paths, we only care about the "simplest" ones. Or + * more formally, the minimum prefixes of all paths. For example, + * if a class is reachable by the following paths: + * + * - a -> b + * - a -> b -> c + * - d -> c + * - d + * + * We really only care about: + * + * - a -> b + * - d + * + * Because if we reach the class through a path that goes through + * `c`, it is necessarily already loaded. + * + * Once we have obtained this minimal set of paths, we use the last + * element of each path to determine the final module + * grouping. This is because these have an actual static dependency + * on the node in question. + * + * Merging these tags into a single `ModuleID` is delegated to the + * caller. + * + * == Class Exclusion == + * Classes can be excluded from the modules generated by FewestModulesTagger. + * + * For excluded classes, the FewestModulesTagger assumes that they are in a module + * provided by a different part of the overall analysis. + * + * The (transitive) dependencies of the class are nevertheless taken into + * account and tagged as appropriate. + * In particular, to avoid cycles and excessive splitting alike (see #4835), + * we need to introduce an additonal tagging mechanism. + * + * To illustrate the problem, take the following dependency graph as an example + * + * a -> b -> c + * + * where B is excluded. Naively, we would want to group a and c together into a'. + * However, this would lead to a circular dependency between a' and c. + * + * Nevertheless, in the absence of b, or if b is not an excluded class, we'd + * want to perform the grouping to avoid unnecessary splitting. + * + * We achieve this by tracking an additional tag, representing the maximum + * number of hops from an excluded class (aka fine) to a non-excluded class + * (aka coarse) class for any path from an entrypoint to the given class. + * + * We then only permit grouping coarse classes with the same tag. This avoids + * the creation of cycles. + * + * The following is a proof that this strategy avoids cycles. + * + * Given + * + * G = (V, E), acyclic, V = F ∪ C, F ∩ C = ∅ + * the original dependency graph, + * F: set of fine classes, + * C: set of coarse classes + * + * t : V → ℕ (the maxExcludedHopCount tag) + * ∀ (v1, v2) ∈ E : t(v1) ≤ t(v2) + * ∀ (f, c) ∈ E : f ∈ F, c ∈ C ⇒ t(f) < t(c) + * + * Define + * + * G' = (V', E'), V' = F ∪ C' (the new grouped graph) + * + * C' = { n ∈ ℕ | ∃ c ∈ C : t(c) = n } + * + * E' = { (f1, f2) ∈ E | f1, f2 ∈ F } ∪ + * { (f, n) | f ∈ F, ∃ c ∈ C : (f, c) ∈ E : t(c) = n } ∪ + * { (n, f) | f ∈ F, ∃ c ∈ C : (c, f) ∈ E : t(c) = n } ∪ + * { (n, m) | n ≠ m, ∃ c1, c2 ∈ C : (c1, c2) ∈ E : t(c1) = n, t(c2) = m } + * + * t' : V' → ℕ: + * + * t'(f) = t(f) (if f ∈ F) + * t'(n) = n (if n ∈ C') + * + * Lemma 1 (unproven) + * + * ∀ (v1, v2) ∈ E' : t'(v1) ≤ t'(v2) + * + * Lemma 2 (unproven) + * + * ∀ (f, n) ∈ E' : f ∈ F, n ∈ C' : t'(f) < t'(n) + * + * Lemma 3 + * + * ∀ (n, m) ∈ E' : n,m ∈ C' ⇒ t'(n) < t'(m) + * + * Follows from Lemma 1 and (n, m) ∈ E' ⇒ n ≠ m (by definition). + * + * Theorem + * + * G' is acyclic + * + * Proof by contradiction. + * + * Assume ∃ p = x1, ..., xn (x1 = xn, n > 1, xi ∈ V) + * + * ∃ xi ∈ C' by contradiction: ∀ xi ∈ F ⇒ p is a cycle in G + * + * ∃ xi ∈ F by contradiction: ∀ xi ∈ C' ⇒ + * t'(xi) increases strictly monotonically (by Lemma 3), + * but x1 = xn ⇒ t'(x1) = t'(xn) + * + * Therefore, + * + * ∃ (xi, xj) ∈ p : xi ∈ F, xj ∈ C' + * + * Therefore (by Lemma 1) + * + * t'(x1) ≤ ... ≤ t'(xi) < t'(xj) ≤ ... ≤ t'(xn) ⇒ t'(x1) < t'(xn) + * + * But x1 = xn ⇒ t'(x1) = t'(xn), which is a contradiction. + * + * Therefore, G' is acyclic. + */ +private class FewestModulesTagger(infos: ModuleAnalyzer.DependencyInfo) { + import FewestModulesTagger._ + + private[this] val allPaths = mutable.Map.empty[ClassName, Paths] + + final def tagAll(modulesToAvoid: Iterable[ModuleID]): scala.collection.Map[ClassName, ModuleID] = { + val internalModIDGenerator = new InternalModuleIDGenerator.ForDigests(modulesToAvoid) + tagEntryPoints() + for { + (className, paths) <- allPaths + } yield { + className -> paths.moduleID(internalModIDGenerator) + } + } + + private def tag(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName]): Unit = { + val updated = allPaths + .getOrElseUpdate(className, new Paths) + .put(pathRoot, pathSteps) + + if (updated) { + val classInfo = infos.classDependencies(className) + classInfo + .staticDependencies + .foreach(staticEdge(_, pathRoot, pathSteps)) + + classInfo + .dynamicDependencies + .foreach(dynamicEdge(_, pathRoot, pathSteps)) + } + } + + private def staticEdge(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName]): Unit = { + tag(className, pathRoot, pathSteps) + } + + private def dynamicEdge(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName]): Unit = { + tag(className, pathRoot, pathSteps :+ className) + } + + private def tagEntryPoints(): Unit = { + for { + (moduleID, deps) <- infos.publicModuleDependencies + className <- deps + } { + staticEdge(className, pathRoot = moduleID, pathSteps = Nil) + } + } +} + +private object FewestModulesTagger { + + /** "Interesting" paths that can lead to a given class. + * + * "Interesting" in this context means: + * - All direct paths from a public dependency. + * - All non-empty, mutually prefix-free paths of dynamic import hops. + */ + private final class Paths { + private val direct = mutable.Set.empty[ModuleID] + private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] + + def put(pathRoot: ModuleID, pathSteps: List[ClassName]): Boolean = { + if (pathSteps.isEmpty) { + direct.add(pathRoot) + } else { + dynamic + .getOrElseUpdate(pathRoot, new DynamicPaths) + .put(pathSteps) + } + } + + def moduleID(internalModIDGenerator: ForDigests): ModuleID = { + if (direct.size == 1 && dynamic.isEmpty) { + /* Class is only used by a single public module. Put it there. + * + * Note that we must not do this if there are any dynamic or excluded + * modules requiring this class. Otherwise, the dynamically loaded module + * will try to import the public module (but importing public modules is + * forbidden). + */ + direct.head + } else { + /* Class is used by multiple public modules and/or dynamic edges. + * Create a module ID grouping it with other classes that have the same + * dependees. + */ + val digestBuilder = new SHA1.DigestBuilder + + // Public modules using this. + for (id <- direct.toList.sortBy(_.id)) + digestBuilder.update(id.id.getBytes(StandardCharsets.UTF_8)) + + // Dynamic modules using this. + for (className <- dynamicEnds) + digestBuilder.updateUTF8String(className.encoded) + + internalModIDGenerator.forDigest(digestBuilder.finalizeDigest()) + } + } + + private def intToBytes(x: Int): Array[Byte] = { + val result = new Array[Byte](4) + val buf = ByteBuffer.wrap(result) + buf.putInt(x) + result + } + + private def dynamicEnds: immutable.SortedSet[ClassName] = { + val builder = immutable.SortedSet.newBuilder[ClassName] + /* We ignore paths that originate in a module that imports this class + * directly: They are irrelevant for the final ID. + * + * However, they are important to ensure we do not attempt to import a + * public module (see the comment in moduleID); therefore, we only filter + * them here. + */ + for ((h, t) <- dynamic if !direct.contains(h)) + t.ends(builder) + builder.result() + } + } + + /** Set of shortest, mutually prefix-free paths of dynamic import hops */ + private final class DynamicPaths { + private val content = mutable.Map.empty[ClassName, DynamicPaths] + + @tailrec + def put(path: List[ClassName]): Boolean = { + val h :: t = path + + if (content.get(h).exists(_.content.isEmpty)) { + // shorter or equal path already exists. + false + } else if (t.isEmpty) { + // the path we put stops here, prune longer paths (if any). + content.put(h, new DynamicPaths) + true + } else { + // there are other paths, recurse. + content + .getOrElseUpdate(h, new DynamicPaths) + .put(t) + } + } + + /** Populates `builder` with the ends of all paths. */ + def ends(builder: mutable.Builder[ClassName, Set[ClassName]]): Unit = { + for ((h, t) <- content) { + if (t.content.isEmpty) + builder += h + else + t.ends(builder) + } + } + } +} diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallModulesForAnalyzer.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallModulesForAnalyzer.scala index da4cd3a519..9a3888964b 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallModulesForAnalyzer.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallModulesForAnalyzer.scala @@ -36,7 +36,7 @@ private final class SmallModulesForAnalyzer( val modulesToAvoid = info.publicModuleDependencies.keys ++ reprToModuleID.values val largeModuleMap = - new Tagger(info, excludedClasses = targetClassToRepr.keySet).tagAll(modulesToAvoid) + new SmallestModulesForTagger(info, excludedClasses = targetClassToRepr.keySet).tagAll(modulesToAvoid) new SmallModulesForAnalyzer.Analysis(targetClassToRepr, reprToModuleID, largeModuleMap) } diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallestModulesForTagger.scala similarity index 95% rename from linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala rename to linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallestModulesForTagger.scala index 8695eef4b9..2e96acd461 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallestModulesForTagger.scala @@ -26,7 +26,7 @@ import org.scalajs.linker.standard.ModuleSet.ModuleID import InternalModuleIDGenerator.ForDigests -/** Tagger groups classes into coarse modules. +/** SmallestModulesForTagger groups classes into coarse modules. * * It is the primary mechanism for the FewestModulesAnalyzer but also used * by the SmallModulesForAnalyzer. @@ -62,9 +62,9 @@ import InternalModuleIDGenerator.ForDigests * caller. * * == Class Exclusion == - * Classes can be excluded from the modules generated by Tagger. + * Classes can be excluded from the modules generated by SmallestModulesForTagger. * - * For excluded classes, the Tagger assumes that they are in a module + * For excluded classes, the SmallestModulesForTagger assumes that they are in a module * provided by a different part of the overall analysis. * * The (transitive) dependencies of the class are nevertheless taken into @@ -158,9 +158,9 @@ import InternalModuleIDGenerator.ForDigests * * Therefore, G' is acyclic. */ -private class Tagger(infos: ModuleAnalyzer.DependencyInfo, - excludedClasses: scala.collection.Set[ClassName] = Set.empty) { - import Tagger._ +private class SmallestModulesForTagger(infos: ModuleAnalyzer.DependencyInfo, + excludedClasses: scala.collection.Set[ClassName]) { + import SmallestModulesForTagger._ private[this] val allPaths = mutable.Map.empty[ClassName, Paths] @@ -220,7 +220,7 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, } } -private object Tagger { +private object SmallestModulesForTagger { /** "Interesting" paths that can lead to a given class. * @@ -229,6 +229,7 @@ private object Tagger { * - All non-empty, mutually prefix-free paths of dynamic import hops. */ private final class Paths { + // FIXME: This seems wrong. It depends on the order that paths are traversed. private var maxExcludedHopCount = 0 private val direct = mutable.Set.empty[ModuleID] private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] From 7fbe85ef37ae9f352687655d13d5d40f817b2ef2 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Mon, 20 May 2024 21:12:25 +1200 Subject: [PATCH 02/16] Fix #4985: Tag FewestModules breadth first --- .../modulesplitter/FewestModulesTagger.scala | 62 +++++++++++-------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala index b95a369f57..b2888c0a3c 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala @@ -169,37 +169,47 @@ private class FewestModulesTagger(infos: ModuleAnalyzer.DependencyInfo) { } } - private def tag(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName]): Unit = { - val updated = allPaths - .getOrElseUpdate(className, new Paths) - .put(pathRoot, pathSteps) - - if (updated) { - val classInfo = infos.classDependencies(className) - classInfo - .staticDependencies - .foreach(staticEdge(_, pathRoot, pathSteps)) - - classInfo - .dynamicDependencies - .foreach(dynamicEdge(_, pathRoot, pathSteps)) + @tailrec + private def tag(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], + dynamics: Set[ClassName]): Set[ClassName] = { + classNames.headOption match { + case Some(className) => + val updated = allPaths + .getOrElseUpdate(className, new Paths) + .put(pathRoot, pathSteps) + if (updated) { + val classInfo = infos.classDependencies(className) + tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, + dynamics ++ classInfo.dynamicDependencies) + } else { + tag(classNames.tail, pathRoot, pathSteps, dynamics) + } + case None => dynamics } } - private def staticEdge(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName]): Unit = { - tag(className, pathRoot, pathSteps) - } - - private def dynamicEdge(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName]): Unit = { - tag(className, pathRoot, pathSteps :+ className) + @tailrec + private def tagDynamics(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], + dynamics: List[(List[ClassName], Set[ClassName])]): Unit = { + classNames.headOption match { + case Some(className) => + val nextPathSteps = pathSteps :+ className + val relativeDynamics = tag(Set(className), pathRoot, nextPathSteps, Set.empty) + val nextDynamics = nextPathSteps -> relativeDynamics :: dynamics + tagDynamics(classNames.tail, pathRoot, pathSteps, nextDynamics) + case None => dynamics match { + case (pathSteps, classNames) :: remainingDynamics => + tagDynamics(classNames, pathRoot, pathSteps, remainingDynamics) + case Nil => () + } + } } private def tagEntryPoints(): Unit = { - for { - (moduleID, deps) <- infos.publicModuleDependencies - className <- deps - } { - staticEdge(className, pathRoot = moduleID, pathSteps = Nil) + infos.publicModuleDependencies.foreach { + case (moduleID, deps) => + val dynamics = tag(classNames = deps, pathRoot = moduleID, pathSteps = Nil, dynamics = Set.empty) + tagDynamics(classNames = dynamics, pathRoot = moduleID, pathSteps = Nil, dynamics = Nil) } } } @@ -208,7 +218,7 @@ private object FewestModulesTagger { /** "Interesting" paths that can lead to a given class. * - * "Interesting" in this context means: + * "Interesting" in this context means: * - All direct paths from a public dependency. * - All non-empty, mutually prefix-free paths of dynamic import hops. */ From 3078faceef3991ac3cc04913535db1b48190eb7e Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Mon, 20 May 2024 23:08:59 +1200 Subject: [PATCH 03/16] Fix #4985: Fix transitive static dependencies --- .../modulesplitter/FewestModulesTagger.scala | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala index b2888c0a3c..68d3daad21 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala @@ -173,17 +173,26 @@ private class FewestModulesTagger(infos: ModuleAnalyzer.DependencyInfo) { private def tag(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], dynamics: Set[ClassName]): Set[ClassName] = { classNames.headOption match { - case Some(className) => - val updated = allPaths - .getOrElseUpdate(className, new Paths) - .put(pathRoot, pathSteps) - if (updated) { + case Some(className) => allPaths.get(className) match { + case Some(paths) if !paths.hasDynamic && pathSteps.nonEmpty => + // Special case that visits static dependencies again when the first dynamic dependency is found so as to + // ensure that they are not thought to only be used by a single public module. + paths.put(pathRoot, pathSteps) + val classInfo = infos.classDependencies(className) + tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, dynamics) + case None => + val paths = new Paths + paths.put(pathRoot, pathSteps) + allPaths.put(className, paths) + // Consider dependencies the first time we encounter them as this is the shortest path there will be. val classInfo = infos.classDependencies(className) tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, dynamics ++ classInfo.dynamicDependencies) - } else { + case Some(paths) => + paths.put(pathRoot, pathSteps) + // Otherwise do not consider dependencies again as there is no more information to find. tag(classNames.tail, pathRoot, pathSteps, dynamics) - } + } case None => dynamics } } @@ -226,6 +235,8 @@ private object FewestModulesTagger { private val direct = mutable.Set.empty[ModuleID] private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] + def hasDynamic: Boolean = dynamic.nonEmpty + def put(pathRoot: ModuleID, pathSteps: List[ClassName]): Boolean = { if (pathSteps.isEmpty) { direct.add(pathRoot) From 310469be24721a07bdbaaf358a50023a7893b8fa Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Mon, 20 May 2024 23:11:46 +1200 Subject: [PATCH 04/16] Fix #4985: Fix docs --- .../modulesplitter/FewestModulesTagger.scala | 99 +------------------ 1 file changed, 1 insertion(+), 98 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala index 68d3daad21..4b9e5a50ff 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala @@ -56,103 +56,6 @@ import scala.collection.{immutable, mutable} * * Merging these tags into a single `ModuleID` is delegated to the * caller. - * - * == Class Exclusion == - * Classes can be excluded from the modules generated by FewestModulesTagger. - * - * For excluded classes, the FewestModulesTagger assumes that they are in a module - * provided by a different part of the overall analysis. - * - * The (transitive) dependencies of the class are nevertheless taken into - * account and tagged as appropriate. - * In particular, to avoid cycles and excessive splitting alike (see #4835), - * we need to introduce an additonal tagging mechanism. - * - * To illustrate the problem, take the following dependency graph as an example - * - * a -> b -> c - * - * where B is excluded. Naively, we would want to group a and c together into a'. - * However, this would lead to a circular dependency between a' and c. - * - * Nevertheless, in the absence of b, or if b is not an excluded class, we'd - * want to perform the grouping to avoid unnecessary splitting. - * - * We achieve this by tracking an additional tag, representing the maximum - * number of hops from an excluded class (aka fine) to a non-excluded class - * (aka coarse) class for any path from an entrypoint to the given class. - * - * We then only permit grouping coarse classes with the same tag. This avoids - * the creation of cycles. - * - * The following is a proof that this strategy avoids cycles. - * - * Given - * - * G = (V, E), acyclic, V = F ∪ C, F ∩ C = ∅ - * the original dependency graph, - * F: set of fine classes, - * C: set of coarse classes - * - * t : V → ℕ (the maxExcludedHopCount tag) - * ∀ (v1, v2) ∈ E : t(v1) ≤ t(v2) - * ∀ (f, c) ∈ E : f ∈ F, c ∈ C ⇒ t(f) < t(c) - * - * Define - * - * G' = (V', E'), V' = F ∪ C' (the new grouped graph) - * - * C' = { n ∈ ℕ | ∃ c ∈ C : t(c) = n } - * - * E' = { (f1, f2) ∈ E | f1, f2 ∈ F } ∪ - * { (f, n) | f ∈ F, ∃ c ∈ C : (f, c) ∈ E : t(c) = n } ∪ - * { (n, f) | f ∈ F, ∃ c ∈ C : (c, f) ∈ E : t(c) = n } ∪ - * { (n, m) | n ≠ m, ∃ c1, c2 ∈ C : (c1, c2) ∈ E : t(c1) = n, t(c2) = m } - * - * t' : V' → ℕ: - * - * t'(f) = t(f) (if f ∈ F) - * t'(n) = n (if n ∈ C') - * - * Lemma 1 (unproven) - * - * ∀ (v1, v2) ∈ E' : t'(v1) ≤ t'(v2) - * - * Lemma 2 (unproven) - * - * ∀ (f, n) ∈ E' : f ∈ F, n ∈ C' : t'(f) < t'(n) - * - * Lemma 3 - * - * ∀ (n, m) ∈ E' : n,m ∈ C' ⇒ t'(n) < t'(m) - * - * Follows from Lemma 1 and (n, m) ∈ E' ⇒ n ≠ m (by definition). - * - * Theorem - * - * G' is acyclic - * - * Proof by contradiction. - * - * Assume ∃ p = x1, ..., xn (x1 = xn, n > 1, xi ∈ V) - * - * ∃ xi ∈ C' by contradiction: ∀ xi ∈ F ⇒ p is a cycle in G - * - * ∃ xi ∈ F by contradiction: ∀ xi ∈ C' ⇒ - * t'(xi) increases strictly monotonically (by Lemma 3), - * but x1 = xn ⇒ t'(x1) = t'(xn) - * - * Therefore, - * - * ∃ (xi, xj) ∈ p : xi ∈ F, xj ∈ C' - * - * Therefore (by Lemma 1) - * - * t'(x1) ≤ ... ≤ t'(xi) < t'(xj) ≤ ... ≤ t'(xn) ⇒ t'(x1) < t'(xn) - * - * But x1 = xn ⇒ t'(x1) = t'(xn), which is a contradiction. - * - * Therefore, G' is acyclic. */ private class FewestModulesTagger(infos: ModuleAnalyzer.DependencyInfo) { import FewestModulesTagger._ @@ -251,7 +154,7 @@ private object FewestModulesTagger { if (direct.size == 1 && dynamic.isEmpty) { /* Class is only used by a single public module. Put it there. * - * Note that we must not do this if there are any dynamic or excluded + * Note that we must not do this if there are any dynamic * modules requiring this class. Otherwise, the dynamically loaded module * will try to import the public module (but importing public modules is * forbidden). From 1644a974e15d7d44ff3c2862a901d6aaf4086a9d Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Tue, 21 May 2024 13:49:21 +1200 Subject: [PATCH 05/16] Fix #4985: Combine taggers again --- .../FewestModulesAnalyzer.scala | 2 +- .../modulesplitter/FewestModulesTagger.scala | 237 ------------------ .../SmallModulesForAnalyzer.scala | 2 +- ...estModulesForTagger.scala => Tagger.scala} | 131 ++++++---- .../frontend/modulesplitter/TaggerTest.scala | 80 ++++++ 5 files changed, 163 insertions(+), 289 deletions(-) delete mode 100644 linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala rename linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/{SmallestModulesForTagger.scala => Tagger.scala} (69%) create mode 100644 linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesAnalyzer.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesAnalyzer.scala index 0a80a01286..5b96f5eaf3 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesAnalyzer.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesAnalyzer.scala @@ -33,7 +33,7 @@ private[modulesplitter] final class FewestModulesAnalyzer extends ModuleAnalyzer new SingleModuleAnalysis(info.publicModuleDependencies.head._1) } else { val modulesToAvoid = info.publicModuleDependencies.keys - val moduleMap = new FewestModulesTagger(info).tagAll(modulesToAvoid) + val moduleMap = new Tagger(info).tagAll(modulesToAvoid) new FullAnalysis(moduleMap) } diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala deleted file mode 100644 index 4b9e5a50ff..0000000000 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/FewestModulesTagger.scala +++ /dev/null @@ -1,237 +0,0 @@ -/* - * Scala.js (https://www.scala-js.org/) - * - * Copyright EPFL. - * - * Licensed under Apache License 2.0 - * (https://www.apache.org/licenses/LICENSE-2.0). - * - * See the NOTICE file distributed with this work for - * additional information regarding copyright ownership. - */ - -package org.scalajs.linker.frontend.modulesplitter - -import org.scalajs.ir.Names.ClassName -import org.scalajs.ir.SHA1 -import org.scalajs.linker.frontend.modulesplitter.InternalModuleIDGenerator.ForDigests -import org.scalajs.linker.standard.ModuleSet.ModuleID - -import java.nio.ByteBuffer -import java.nio.charset.StandardCharsets -import scala.annotation.tailrec -import scala.collection.{immutable, mutable} - -/** FewestModulesTagger groups classes into coarse modules. - * - * It is the primary mechanism for the FewestModulesAnalyzer but also used - * by the SmallModulesForAnalyzer. - * - * To group classes into modules appropriately, we want to know for - * each class, "how" it can be reached. In practice, this means we - * record the path from the original public module and every - * dynamic import hop we made. - * - * Of all these paths, we only care about the "simplest" ones. Or - * more formally, the minimum prefixes of all paths. For example, - * if a class is reachable by the following paths: - * - * - a -> b - * - a -> b -> c - * - d -> c - * - d - * - * We really only care about: - * - * - a -> b - * - d - * - * Because if we reach the class through a path that goes through - * `c`, it is necessarily already loaded. - * - * Once we have obtained this minimal set of paths, we use the last - * element of each path to determine the final module - * grouping. This is because these have an actual static dependency - * on the node in question. - * - * Merging these tags into a single `ModuleID` is delegated to the - * caller. - */ -private class FewestModulesTagger(infos: ModuleAnalyzer.DependencyInfo) { - import FewestModulesTagger._ - - private[this] val allPaths = mutable.Map.empty[ClassName, Paths] - - final def tagAll(modulesToAvoid: Iterable[ModuleID]): scala.collection.Map[ClassName, ModuleID] = { - val internalModIDGenerator = new InternalModuleIDGenerator.ForDigests(modulesToAvoid) - tagEntryPoints() - for { - (className, paths) <- allPaths - } yield { - className -> paths.moduleID(internalModIDGenerator) - } - } - - @tailrec - private def tag(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], - dynamics: Set[ClassName]): Set[ClassName] = { - classNames.headOption match { - case Some(className) => allPaths.get(className) match { - case Some(paths) if !paths.hasDynamic && pathSteps.nonEmpty => - // Special case that visits static dependencies again when the first dynamic dependency is found so as to - // ensure that they are not thought to only be used by a single public module. - paths.put(pathRoot, pathSteps) - val classInfo = infos.classDependencies(className) - tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, dynamics) - case None => - val paths = new Paths - paths.put(pathRoot, pathSteps) - allPaths.put(className, paths) - // Consider dependencies the first time we encounter them as this is the shortest path there will be. - val classInfo = infos.classDependencies(className) - tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, - dynamics ++ classInfo.dynamicDependencies) - case Some(paths) => - paths.put(pathRoot, pathSteps) - // Otherwise do not consider dependencies again as there is no more information to find. - tag(classNames.tail, pathRoot, pathSteps, dynamics) - } - case None => dynamics - } - } - - @tailrec - private def tagDynamics(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], - dynamics: List[(List[ClassName], Set[ClassName])]): Unit = { - classNames.headOption match { - case Some(className) => - val nextPathSteps = pathSteps :+ className - val relativeDynamics = tag(Set(className), pathRoot, nextPathSteps, Set.empty) - val nextDynamics = nextPathSteps -> relativeDynamics :: dynamics - tagDynamics(classNames.tail, pathRoot, pathSteps, nextDynamics) - case None => dynamics match { - case (pathSteps, classNames) :: remainingDynamics => - tagDynamics(classNames, pathRoot, pathSteps, remainingDynamics) - case Nil => () - } - } - } - - private def tagEntryPoints(): Unit = { - infos.publicModuleDependencies.foreach { - case (moduleID, deps) => - val dynamics = tag(classNames = deps, pathRoot = moduleID, pathSteps = Nil, dynamics = Set.empty) - tagDynamics(classNames = dynamics, pathRoot = moduleID, pathSteps = Nil, dynamics = Nil) - } - } -} - -private object FewestModulesTagger { - - /** "Interesting" paths that can lead to a given class. - * - * "Interesting" in this context means: - * - All direct paths from a public dependency. - * - All non-empty, mutually prefix-free paths of dynamic import hops. - */ - private final class Paths { - private val direct = mutable.Set.empty[ModuleID] - private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] - - def hasDynamic: Boolean = dynamic.nonEmpty - - def put(pathRoot: ModuleID, pathSteps: List[ClassName]): Boolean = { - if (pathSteps.isEmpty) { - direct.add(pathRoot) - } else { - dynamic - .getOrElseUpdate(pathRoot, new DynamicPaths) - .put(pathSteps) - } - } - - def moduleID(internalModIDGenerator: ForDigests): ModuleID = { - if (direct.size == 1 && dynamic.isEmpty) { - /* Class is only used by a single public module. Put it there. - * - * Note that we must not do this if there are any dynamic - * modules requiring this class. Otherwise, the dynamically loaded module - * will try to import the public module (but importing public modules is - * forbidden). - */ - direct.head - } else { - /* Class is used by multiple public modules and/or dynamic edges. - * Create a module ID grouping it with other classes that have the same - * dependees. - */ - val digestBuilder = new SHA1.DigestBuilder - - // Public modules using this. - for (id <- direct.toList.sortBy(_.id)) - digestBuilder.update(id.id.getBytes(StandardCharsets.UTF_8)) - - // Dynamic modules using this. - for (className <- dynamicEnds) - digestBuilder.updateUTF8String(className.encoded) - - internalModIDGenerator.forDigest(digestBuilder.finalizeDigest()) - } - } - - private def intToBytes(x: Int): Array[Byte] = { - val result = new Array[Byte](4) - val buf = ByteBuffer.wrap(result) - buf.putInt(x) - result - } - - private def dynamicEnds: immutable.SortedSet[ClassName] = { - val builder = immutable.SortedSet.newBuilder[ClassName] - /* We ignore paths that originate in a module that imports this class - * directly: They are irrelevant for the final ID. - * - * However, they are important to ensure we do not attempt to import a - * public module (see the comment in moduleID); therefore, we only filter - * them here. - */ - for ((h, t) <- dynamic if !direct.contains(h)) - t.ends(builder) - builder.result() - } - } - - /** Set of shortest, mutually prefix-free paths of dynamic import hops */ - private final class DynamicPaths { - private val content = mutable.Map.empty[ClassName, DynamicPaths] - - @tailrec - def put(path: List[ClassName]): Boolean = { - val h :: t = path - - if (content.get(h).exists(_.content.isEmpty)) { - // shorter or equal path already exists. - false - } else if (t.isEmpty) { - // the path we put stops here, prune longer paths (if any). - content.put(h, new DynamicPaths) - true - } else { - // there are other paths, recurse. - content - .getOrElseUpdate(h, new DynamicPaths) - .put(t) - } - } - - /** Populates `builder` with the ends of all paths. */ - def ends(builder: mutable.Builder[ClassName, Set[ClassName]]): Unit = { - for ((h, t) <- content) { - if (t.content.isEmpty) - builder += h - else - t.ends(builder) - } - } - } -} diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallModulesForAnalyzer.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallModulesForAnalyzer.scala index 9a3888964b..da4cd3a519 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallModulesForAnalyzer.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallModulesForAnalyzer.scala @@ -36,7 +36,7 @@ private final class SmallModulesForAnalyzer( val modulesToAvoid = info.publicModuleDependencies.keys ++ reprToModuleID.values val largeModuleMap = - new SmallestModulesForTagger(info, excludedClasses = targetClassToRepr.keySet).tagAll(modulesToAvoid) + new Tagger(info, excludedClasses = targetClassToRepr.keySet).tagAll(modulesToAvoid) new SmallModulesForAnalyzer.Analysis(targetClassToRepr, reprToModuleID, largeModuleMap) } diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallestModulesForTagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala similarity index 69% rename from linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallestModulesForTagger.scala rename to linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index 2e96acd461..365b3dcf01 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/SmallestModulesForTagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -12,21 +12,17 @@ package org.scalajs.linker.frontend.modulesplitter -import scala.annotation.tailrec - -import scala.collection.immutable -import scala.collection.mutable - -import java.nio.charset.StandardCharsets -import java.nio.ByteBuffer - import org.scalajs.ir.Names.ClassName import org.scalajs.ir.SHA1 +import org.scalajs.linker.frontend.modulesplitter.InternalModuleIDGenerator.ForDigests import org.scalajs.linker.standard.ModuleSet.ModuleID -import InternalModuleIDGenerator.ForDigests +import java.nio.ByteBuffer +import java.nio.charset.StandardCharsets +import scala.annotation.tailrec +import scala.collection.{immutable, mutable} -/** SmallestModulesForTagger groups classes into coarse modules. +/** Tagger groups classes into coarse modules. * * It is the primary mechanism for the FewestModulesAnalyzer but also used * by the SmallModulesForAnalyzer. @@ -158,9 +154,9 @@ import InternalModuleIDGenerator.ForDigests * * Therefore, G' is acyclic. */ -private class SmallestModulesForTagger(infos: ModuleAnalyzer.DependencyInfo, - excludedClasses: scala.collection.Set[ClassName]) { - import SmallestModulesForTagger._ +private class Tagger(infos: ModuleAnalyzer.DependencyInfo, + excludedClasses: scala.collection.Set[ClassName] = Set.empty) { + import Tagger._ private[this] val allPaths = mutable.Map.empty[ClassName, Paths] @@ -169,89 +165,124 @@ private class SmallestModulesForTagger(infos: ModuleAnalyzer.DependencyInfo, tagEntryPoints() for { (className, paths) <- allPaths - if !excludedClasses.contains(className) } yield { className -> paths.moduleID(internalModIDGenerator) } } - private def tag(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName], - excludedHopCount: Int, fromExcluded: Boolean): Unit = { + @tailrec + private def tag(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], + dynamics: Set[ClassName]): Set[ClassName] = { + classNames.headOption match { + case Some(className) => allPaths.get(className) match { + case Some(paths) if !paths.hasDynamic && pathSteps.nonEmpty => + // Special case that visits static dependencies again when the first dynamic dependency is found so as to + // ensure that they are not thought to only be used by a single public module. + paths.put(pathRoot, pathSteps) + val classInfo = infos.classDependencies(className) + tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, dynamics) + case None => + val paths = new Paths + paths.put(pathRoot, pathSteps) + allPaths.put(className, paths) + // Consider dependencies the first time we encounter them as this is the shortest path there will be. + val classInfo = infos.classDependencies(className) + tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, + dynamics ++ classInfo.dynamicDependencies) + case Some(paths) => + paths.put(pathRoot, pathSteps) + // Otherwise do not consider dependencies again as there is no more information to find. + tag(classNames.tail, pathRoot, pathSteps, dynamics) + } + case None => dynamics + } + } + + @tailrec + private def tagDynamics(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], + dynamics: List[(List[ClassName], Set[ClassName])]): Unit = { + classNames.headOption match { + case Some(className) => + val nextPathSteps = pathSteps :+ className + val relativeDynamics = tag(Set(className), pathRoot, nextPathSteps, Set.empty) + val nextDynamics = nextPathSteps -> relativeDynamics :: dynamics + tagDynamics(classNames.tail, pathRoot, pathSteps, nextDynamics) + case None => dynamics match { + case (pathSteps, classNames) :: remainingDynamics => + tagDynamics(classNames, pathRoot, pathSteps, remainingDynamics) + case Nil => () + } + } + } + + private def updateExcludedHopCounts(className: ClassName, excludedHopCount: Int, fromExcluded: Boolean): Unit = { val isExcluded = excludedClasses.contains(className) val newExcludedHopCount = if (fromExcluded && !isExcluded) excludedHopCount + 1 // hop from fine to coarse else excludedHopCount - val updated = allPaths - .getOrElseUpdate(className, new Paths) - .put(pathRoot, pathSteps, newExcludedHopCount) + val updated = allPaths(className).updateExcludedHopCount(excludedHopCount) if (updated) { val classInfo = infos.classDependencies(className) classInfo .staticDependencies - .foreach(staticEdge(_, pathRoot, pathSteps, newExcludedHopCount, fromExcluded = isExcluded)) - + .foreach(updateExcludedHopCounts(_, newExcludedHopCount, fromExcluded = isExcluded)) classInfo .dynamicDependencies - .foreach(dynamicEdge(_, pathRoot, pathSteps, newExcludedHopCount, fromExcluded = isExcluded)) + .foreach(updateExcludedHopCounts(_, newExcludedHopCount, fromExcluded = isExcluded)) } } - private def staticEdge(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName], - excludedHopCount: Int, fromExcluded: Boolean): Unit = { - tag(className, pathRoot, pathSteps, excludedHopCount, fromExcluded) - } - - private def dynamicEdge(className: ClassName, pathRoot: ModuleID, pathSteps: List[ClassName], - excludedHopCount: Int, fromExcluded: Boolean): Unit = { - tag(className, pathRoot, pathSteps :+ className, excludedHopCount, fromExcluded) - } - private def tagEntryPoints(): Unit = { - for { - (moduleID, deps) <- infos.publicModuleDependencies - className <- deps - } { - staticEdge(className, pathRoot = moduleID, pathSteps = Nil, - excludedHopCount = 0, fromExcluded = false) + infos.publicModuleDependencies.foreach { + case (moduleID, deps) => + // We need to be careful with memory usage here. There is a contention between finding the shortest path and + // finding the maximum excluded hop count. For the former it is best to do a breadth first traversal but for the + // later we do a depth first traversal. + val dynamics = tag(classNames = deps, pathRoot = moduleID, pathSteps = Nil, dynamics = Set.empty) + tagDynamics(classNames = dynamics, pathRoot = moduleID, pathSteps = Nil, dynamics = Nil) + deps.foreach(updateExcludedHopCounts(_, excludedHopCount = 0, fromExcluded = false)) } } } -private object SmallestModulesForTagger { +private object Tagger { /** "Interesting" paths that can lead to a given class. * - * "Interesting" in this context means: + * "Interesting" in this context means: * - All direct paths from a public dependency. * - All non-empty, mutually prefix-free paths of dynamic import hops. */ private final class Paths { - // FIXME: This seems wrong. It depends on the order that paths are traversed. - private var maxExcludedHopCount = 0 + private var maxExcludedHopCount = -1 private val direct = mutable.Set.empty[ModuleID] private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] - def put(pathRoot: ModuleID, pathSteps: List[ClassName], excludedHopCount: Int): Boolean = { - val hopCountsChanged = excludedHopCount > maxExcludedHopCount - - if (hopCountsChanged) - maxExcludedHopCount = excludedHopCount + def hasDynamic: Boolean = dynamic.nonEmpty - val stepsChanged = if (pathSteps.isEmpty) { + def put(pathRoot: ModuleID, pathSteps: List[ClassName]): Unit = { + if (pathSteps.isEmpty) { direct.add(pathRoot) } else { dynamic .getOrElseUpdate(pathRoot, new DynamicPaths) .put(pathSteps) } - hopCountsChanged || stepsChanged + } + + def updateExcludedHopCount(excludedHopCount: Int): Boolean = { + val hopCountsChanged = excludedHopCount > maxExcludedHopCount + if (hopCountsChanged) + maxExcludedHopCount = excludedHopCount + hopCountsChanged } def moduleID(internalModIDGenerator: ForDigests): ModuleID = { - if (direct.size == 1 && dynamic.isEmpty && maxExcludedHopCount == 0) { + assert(maxExcludedHopCount >= 0, "Maximum excluded hop count has not been calculated") + if (direct.size == 1 && dynamic.isEmpty && maxExcludedHopCount <= 0) { /* Class is only used by a single public module. Put it there. * * Note that we must not do this if there are any dynamic or excluded diff --git a/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala new file mode 100644 index 0000000000..b9ebd723c7 --- /dev/null +++ b/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala @@ -0,0 +1,80 @@ +/* + * Scala.js (https://www.scala-js.org/) + * + * Copyright EPFL. + * + * Licensed under Apache License 2.0 + * (https://www.apache.org/licenses/LICENSE-2.0). + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +package org.scalajs.linker.frontend.modulesplitter + +import org.junit.Assert._ +import org.junit.Test +import org.scalajs.ir.Names.ClassName +import org.scalajs.linker.frontend.modulesplitter.ModuleAnalyzer.{ClassInfo, DependencyInfo} +import org.scalajs.linker.standard.ModuleSet.ModuleID + +import scala.collection.immutable.{ListMap, ListSet} + +class TaggerTest { + + @Test def testLinkedDynamics(): Unit = { + val mainModuleID = ModuleID("main") + + val publicClassName = ClassName("public") + + val routeClassNames = (1 to 100).map(n => ClassName(s"route$n")).toSet + + val classDependencies = ListMap( + publicClassName -> new ClassInfo(Set.empty, routeClassNames) + ) ++ routeClassNames.map(className => className -> new ClassInfo(Set.empty, routeClassNames - className)) + + val publicModuleDependencies = ListMap(mainModuleID -> ListSet(publicClassName)) + + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + + val excludedClasses = Set.empty[ClassName] + + val tagger = new Tagger(info, excludedClasses) + + val moduleMap = tagger.tagAll(Iterable.empty) + + val subjectModuleID = moduleMap(publicClassName) + assertSame(mainModuleID, subjectModuleID) + } + + @Test def testMaxExcludedHopCount(): Unit = { + val mainModuleID = ModuleID("main") + + val publicClassName = ClassName("public") + val forkClassName = ClassName("fork") + val excludedClassName = ClassName("excluded") + val nonExcludedClassName = ClassName("non-excluded") + val subjectClassName = ClassName("subject") + + val classDependencies = ListMap( + publicClassName -> new ClassInfo(ListSet(forkClassName), Set.empty), + forkClassName -> new ClassInfo(ListSet(nonExcludedClassName, excludedClassName), Set.empty), + excludedClassName -> new ClassInfo(ListSet(nonExcludedClassName), Set.empty), + nonExcludedClassName -> new ClassInfo(ListSet(subjectClassName), Set.empty), + subjectClassName -> new ClassInfo(ListSet.empty, Set.empty) + ) + + val publicModuleDependencies = ListMap(mainModuleID -> ListSet(publicClassName)) + + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + + val excludedClasses = Set(excludedClassName) + + val tagger = new Tagger(info, excludedClasses) + + val moduleMap = tagger.tagAll(Iterable.empty) + + val subjectModuleID = moduleMap(subjectClassName) + assertSame(mainModuleID, subjectModuleID) + } +} From c134cf8cf725fc7f6c3e67ae9987e984e0b9a296 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Tue, 21 May 2024 14:31:46 +1200 Subject: [PATCH 06/16] Fix #4985: Update docs --- .../frontend/modulesplitter/Tagger.scala | 67 ++++++++++++++----- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index 365b3dcf01..c853861dad 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -66,7 +66,7 @@ import scala.collection.{immutable, mutable} * The (transitive) dependencies of the class are nevertheless taken into * account and tagged as appropriate. * In particular, to avoid cycles and excessive splitting alike (see #4835), - * we need to introduce an additonal tagging mechanism. + * we need to introduce an additional tagging mechanism. * * To illustrate the problem, take the following dependency graph as an example * @@ -170,9 +170,23 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, } } + /** + * Tags all of the given classes with the given path iff it is shorter than the existing path. + * + * This will recursively tag all static dependencies (which by definition do not contribute to the path). + * + * A class that only has a direct path may be re-tagged with a dynamic path. In this case, all its dependencies are + * also re-tagged where appropriate. + * + * @param classNames the starting set of classes to tag + * @param pathRoot the root of the path + * @param pathSteps the steps that make up path + * @param nextSteps the accumulated result + * @return the next steps to traverse + */ @tailrec private def tag(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], - dynamics: Set[ClassName]): Set[ClassName] = { + nextSteps: Set[ClassName]): Set[ClassName] = { classNames.headOption match { case Some(className) => allPaths.get(className) match { case Some(paths) if !paths.hasDynamic && pathSteps.nonEmpty => @@ -180,7 +194,7 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, // ensure that they are not thought to only be used by a single public module. paths.put(pathRoot, pathSteps) val classInfo = infos.classDependencies(className) - tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, dynamics) + tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, nextSteps) case None => val paths = new Paths paths.put(pathRoot, pathSteps) @@ -188,33 +202,53 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, // Consider dependencies the first time we encounter them as this is the shortest path there will be. val classInfo = infos.classDependencies(className) tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, - dynamics ++ classInfo.dynamicDependencies) + nextSteps ++ classInfo.dynamicDependencies) case Some(paths) => paths.put(pathRoot, pathSteps) // Otherwise do not consider dependencies again as there is no more information to find. - tag(classNames.tail, pathRoot, pathSteps, dynamics) + tag(classNames.tail, pathRoot, pathSteps, nextSteps) } - case None => dynamics + case None => nextSteps } } + /** + * Tags each step relative to the current path and tags them. + * + * Once all of the given steps (and their static dependencies) have been tagged it repeats on the next steps until all + * dependencies have been tagged. + * + * @param classNames the steps to tag and traverse + * @param pathRoot the root of the path + * @param pathSteps the steps that make up path + * @param acc the accumulator + */ @tailrec - private def tagDynamics(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], - dynamics: List[(List[ClassName], Set[ClassName])]): Unit = { + private def tagNextSteps(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], + acc: List[(List[ClassName], Set[ClassName])]): Unit = { classNames.headOption match { case Some(className) => val nextPathSteps = pathSteps :+ className - val relativeDynamics = tag(Set(className), pathRoot, nextPathSteps, Set.empty) - val nextDynamics = nextPathSteps -> relativeDynamics :: dynamics - tagDynamics(classNames.tail, pathRoot, pathSteps, nextDynamics) - case None => dynamics match { - case (pathSteps, classNames) :: remainingDynamics => - tagDynamics(classNames, pathRoot, pathSteps, remainingDynamics) + val nextSteps = tag(Set(className), pathRoot, nextPathSteps, Set.empty) + val nextAcc = nextPathSteps -> nextSteps :: acc + tagNextSteps(classNames.tail, pathRoot, pathSteps, nextAcc) + case None => acc match { + case (pathSteps, classNames) :: nextAcc => + tagNextSteps(classNames, pathRoot, pathSteps, nextAcc) case Nil => () } } } + /** + * Performs a full traversal of the dependencies to re-tag the paths with the maximum excluded hop count. + * + * This will traverse dependencies repeatedly if a prefix is found with a larger excluded hop count. + * + * @param className the starting class to tag + * @param excludedHopCount the excluded hop count so far + * @param fromExcluded whether the previous step was excluded + */ private def updateExcludedHopCounts(className: ClassName, excludedHopCount: Int, fromExcluded: Boolean): Unit = { val isExcluded = excludedClasses.contains(className) @@ -241,8 +275,8 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, // We need to be careful with memory usage here. There is a contention between finding the shortest path and // finding the maximum excluded hop count. For the former it is best to do a breadth first traversal but for the // later we do a depth first traversal. - val dynamics = tag(classNames = deps, pathRoot = moduleID, pathSteps = Nil, dynamics = Set.empty) - tagDynamics(classNames = dynamics, pathRoot = moduleID, pathSteps = Nil, dynamics = Nil) + val nextSteps = tag(classNames = deps, pathRoot = moduleID, pathSteps = Nil, nextSteps = Set.empty) + tagNextSteps(classNames = nextSteps, pathRoot = moduleID, pathSteps = Nil, acc = Nil) deps.foreach(updateExcludedHopCounts(_, excludedHopCount = 0, fromExcluded = false)) } } @@ -257,6 +291,7 @@ private object Tagger { * - All non-empty, mutually prefix-free paths of dynamic import hops. */ private final class Paths { + // Start at -1 so that when we re-tag we consider the first time it is set to 0 as an update. private var maxExcludedHopCount = -1 private val direct = mutable.Set.empty[ModuleID] private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] From a86e07e0c8dd922598b3141fa9a87c2971b0615f Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Tue, 21 May 2024 14:51:57 +1200 Subject: [PATCH 07/16] Fix #4985: Update docs --- .../org/scalajs/linker/frontend/modulesplitter/Tagger.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index c853861dad..3b18965492 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -58,9 +58,9 @@ import scala.collection.{immutable, mutable} * caller. * * == Class Exclusion == - * Classes can be excluded from the modules generated by SmallestModulesForTagger. + * Classes can be excluded from the modules generated by Tagger. * - * For excluded classes, the SmallestModulesForTagger assumes that they are in a module + * For excluded classes, the Tagger assumes that they are in a module * provided by a different part of the overall analysis. * * The (transitive) dependencies of the class are nevertheless taken into @@ -286,7 +286,7 @@ private object Tagger { /** "Interesting" paths that can lead to a given class. * - * "Interesting" in this context means: + * "Interesting" in this context means: * - All direct paths from a public dependency. * - All non-empty, mutually prefix-free paths of dynamic import hops. */ From 96036ad09bee47eb9d4fc1e7b82e13e9b0671481 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Tue, 21 May 2024 14:55:30 +1200 Subject: [PATCH 08/16] Fix #4985: Remove bogus tests --- .../frontend/modulesplitter/TaggerTest.scala | 80 ------------------- 1 file changed, 80 deletions(-) delete mode 100644 linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala diff --git a/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala deleted file mode 100644 index b9ebd723c7..0000000000 --- a/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Scala.js (https://www.scala-js.org/) - * - * Copyright EPFL. - * - * Licensed under Apache License 2.0 - * (https://www.apache.org/licenses/LICENSE-2.0). - * - * See the NOTICE file distributed with this work for - * additional information regarding copyright ownership. - */ - -package org.scalajs.linker.frontend.modulesplitter - -import org.junit.Assert._ -import org.junit.Test -import org.scalajs.ir.Names.ClassName -import org.scalajs.linker.frontend.modulesplitter.ModuleAnalyzer.{ClassInfo, DependencyInfo} -import org.scalajs.linker.standard.ModuleSet.ModuleID - -import scala.collection.immutable.{ListMap, ListSet} - -class TaggerTest { - - @Test def testLinkedDynamics(): Unit = { - val mainModuleID = ModuleID("main") - - val publicClassName = ClassName("public") - - val routeClassNames = (1 to 100).map(n => ClassName(s"route$n")).toSet - - val classDependencies = ListMap( - publicClassName -> new ClassInfo(Set.empty, routeClassNames) - ) ++ routeClassNames.map(className => className -> new ClassInfo(Set.empty, routeClassNames - className)) - - val publicModuleDependencies = ListMap(mainModuleID -> ListSet(publicClassName)) - - val info = new DependencyInfo(classDependencies, publicModuleDependencies) - - val excludedClasses = Set.empty[ClassName] - - val tagger = new Tagger(info, excludedClasses) - - val moduleMap = tagger.tagAll(Iterable.empty) - - val subjectModuleID = moduleMap(publicClassName) - assertSame(mainModuleID, subjectModuleID) - } - - @Test def testMaxExcludedHopCount(): Unit = { - val mainModuleID = ModuleID("main") - - val publicClassName = ClassName("public") - val forkClassName = ClassName("fork") - val excludedClassName = ClassName("excluded") - val nonExcludedClassName = ClassName("non-excluded") - val subjectClassName = ClassName("subject") - - val classDependencies = ListMap( - publicClassName -> new ClassInfo(ListSet(forkClassName), Set.empty), - forkClassName -> new ClassInfo(ListSet(nonExcludedClassName, excludedClassName), Set.empty), - excludedClassName -> new ClassInfo(ListSet(nonExcludedClassName), Set.empty), - nonExcludedClassName -> new ClassInfo(ListSet(subjectClassName), Set.empty), - subjectClassName -> new ClassInfo(ListSet.empty, Set.empty) - ) - - val publicModuleDependencies = ListMap(mainModuleID -> ListSet(publicClassName)) - - val info = new DependencyInfo(classDependencies, publicModuleDependencies) - - val excludedClasses = Set(excludedClassName) - - val tagger = new Tagger(info, excludedClasses) - - val moduleMap = tagger.tagAll(Iterable.empty) - - val subjectModuleID = moduleMap(subjectClassName) - assertSame(mainModuleID, subjectModuleID) - } -} From 4fdba312d59e75ae3e114a789f91f0ea624e7d50 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Wed, 22 May 2024 13:13:19 +1200 Subject: [PATCH 09/16] Skip calculating excluded hops for fewest modules --- .../linker/frontend/modulesplitter/Tagger.scala | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index 3b18965492..79fdce1ab6 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -196,7 +196,7 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, val classInfo = infos.classDependencies(className) tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, nextSteps) case None => - val paths = new Paths + val paths = new Paths(trackExcludedHopCounts = excludedClasses.nonEmpty) paths.put(pathRoot, pathSteps) allPaths.put(className, paths) // Consider dependencies the first time we encounter them as this is the shortest path there will be. @@ -277,7 +277,10 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, // later we do a depth first traversal. val nextSteps = tag(classNames = deps, pathRoot = moduleID, pathSteps = Nil, nextSteps = Set.empty) tagNextSteps(classNames = nextSteps, pathRoot = moduleID, pathSteps = Nil, acc = Nil) - deps.foreach(updateExcludedHopCounts(_, excludedHopCount = 0, fromExcluded = false)) + // Only needed when there are excluded classes. + if (excludedClasses.nonEmpty) { + deps.foreach(updateExcludedHopCounts(_, excludedHopCount = 0, fromExcluded = false)) + } } } } @@ -290,9 +293,9 @@ private object Tagger { * - All direct paths from a public dependency. * - All non-empty, mutually prefix-free paths of dynamic import hops. */ - private final class Paths { + private final class Paths(trackExcludedHopCounts: Boolean) { // Start at -1 so that when we re-tag we consider the first time it is set to 0 as an update. - private var maxExcludedHopCount = -1 + private var maxExcludedHopCount = if (trackExcludedHopCounts) -1 else 0 private val direct = mutable.Set.empty[ModuleID] private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] From bdfe51fdfe0d5e3eacefc11628dbf2819c42583b Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Wed, 22 May 2024 15:13:56 +1200 Subject: [PATCH 10/16] Add missing exclusion check back in --- .../org/scalajs/linker/frontend/modulesplitter/Tagger.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index 79fdce1ab6..a3d28c764a 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -165,6 +165,7 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, tagEntryPoints() for { (className, paths) <- allPaths + if !excludedClasses.contains(className) } yield { className -> paths.moduleID(internalModIDGenerator) } From db851c480240d869e85e0f71d1a394551308b97b Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Wed, 22 May 2024 15:44:35 +1200 Subject: [PATCH 11/16] Fix excluded hop counts --- .../frontend/modulesplitter/Tagger.scala | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index a3d28c764a..407e3f366b 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -190,24 +190,26 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, nextSteps: Set[ClassName]): Set[ClassName] = { classNames.headOption match { case Some(className) => allPaths.get(className) match { - case Some(paths) if !paths.hasDynamic && pathSteps.nonEmpty => - // Special case that visits static dependencies again when the first dynamic dependency is found so as to - // ensure that they are not thought to only be used by a single public module. - paths.put(pathRoot, pathSteps) - val classInfo = infos.classDependencies(className) - tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, nextSteps) case None => val paths = new Paths(trackExcludedHopCounts = excludedClasses.nonEmpty) paths.put(pathRoot, pathSteps) - allPaths.put(className, paths) + allPaths.update(className, paths) // Consider dependencies the first time we encounter them as this is the shortest path there will be. val classInfo = infos.classDependencies(className) tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, nextSteps ++ classInfo.dynamicDependencies) case Some(paths) => - paths.put(pathRoot, pathSteps) - // Otherwise do not consider dependencies again as there is no more information to find. - tag(classNames.tail, pathRoot, pathSteps, nextSteps) + val reprocessStatics = paths.put(pathRoot, pathSteps) + val nextClassNames = + if (reprocessStatics) { + // Revisit static dependencies again when we first detect that this is no longer used by a single public + // module. + classNames.tail ++ infos.classDependencies(className).staticDependencies + } else { + // Otherwise do not consider dependencies again as there is no more information to find. + classNames.tail + } + tag(nextClassNames, pathRoot, pathSteps, nextSteps) } case None => nextSteps } @@ -257,7 +259,7 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, if (fromExcluded && !isExcluded) excludedHopCount + 1 // hop from fine to coarse else excludedHopCount - val updated = allPaths(className).updateExcludedHopCount(excludedHopCount) + val updated = allPaths(className).updateExcludedHopCount(newExcludedHopCount) if (updated) { val classInfo = infos.classDependencies(className) @@ -300,9 +302,14 @@ private object Tagger { private val direct = mutable.Set.empty[ModuleID] private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] - def hasDynamic: Boolean = dynamic.nonEmpty + private def usedBySinglePublicModule: Boolean = + direct.size == 1 && dynamic.isEmpty && maxExcludedHopCount <= 0 - def put(pathRoot: ModuleID, pathSteps: List[ClassName]): Unit = { + /** + * @return `true` iff this was previously only used by a single public dependency but now it is not + */ + def put(pathRoot: ModuleID, pathSteps: List[ClassName]): Boolean = { + val wasUsedBySinglePublicModule = usedBySinglePublicModule if (pathSteps.isEmpty) { direct.add(pathRoot) } else { @@ -310,6 +317,7 @@ private object Tagger { .getOrElseUpdate(pathRoot, new DynamicPaths) .put(pathSteps) } + wasUsedBySinglePublicModule && !usedBySinglePublicModule } def updateExcludedHopCount(excludedHopCount: Int): Boolean = { @@ -321,7 +329,7 @@ private object Tagger { def moduleID(internalModIDGenerator: ForDigests): ModuleID = { assert(maxExcludedHopCount >= 0, "Maximum excluded hop count has not been calculated") - if (direct.size == 1 && dynamic.isEmpty && maxExcludedHopCount <= 0) { + if (usedBySinglePublicModule) { /* Class is only used by a single public module. Put it there. * * Note that we must not do this if there are any dynamic or excluded From b7d95776038f706f4a983fc13e522461335c2ef2 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Wed, 22 May 2024 16:27:59 +1200 Subject: [PATCH 12/16] Fix revisting static dependencies --- .../frontend/modulesplitter/Tagger.scala | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index 407e3f366b..2aa704ab62 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -199,11 +199,13 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, nextSteps ++ classInfo.dynamicDependencies) case Some(paths) => - val reprocessStatics = paths.put(pathRoot, pathSteps) + val moduleIDChanged = paths.put(pathRoot, pathSteps) val nextClassNames = - if (reprocessStatics) { - // Revisit static dependencies again when we first detect that this is no longer used by a single public - // module. + if (moduleIDChanged) { + // TODO: What about dynamic dependencies? I think we need to revisit them too + // Revisit static dependencies again when the module id changes. + // We do not need to revisit dynamic dependencies because by definition they are not used by public + // modules, the first time we tag them is the shortest path so that path end never changes classNames.tail ++ infos.classDependencies(className).staticDependencies } else { // Otherwise do not consider dependencies again as there is no more information to find. @@ -302,14 +304,10 @@ private object Tagger { private val direct = mutable.Set.empty[ModuleID] private val dynamic = mutable.Map.empty[ModuleID, DynamicPaths] - private def usedBySinglePublicModule: Boolean = - direct.size == 1 && dynamic.isEmpty && maxExcludedHopCount <= 0 - /** - * @return `true` iff this was previously only used by a single public dependency but now it is not + * @return `true` if the moduleID for this class changed */ def put(pathRoot: ModuleID, pathSteps: List[ClassName]): Boolean = { - val wasUsedBySinglePublicModule = usedBySinglePublicModule if (pathSteps.isEmpty) { direct.add(pathRoot) } else { @@ -317,7 +315,6 @@ private object Tagger { .getOrElseUpdate(pathRoot, new DynamicPaths) .put(pathSteps) } - wasUsedBySinglePublicModule && !usedBySinglePublicModule } def updateExcludedHopCount(excludedHopCount: Int): Boolean = { @@ -329,7 +326,7 @@ private object Tagger { def moduleID(internalModIDGenerator: ForDigests): ModuleID = { assert(maxExcludedHopCount >= 0, "Maximum excluded hop count has not been calculated") - if (usedBySinglePublicModule) { + if (direct.size == 1 && dynamic.isEmpty && maxExcludedHopCount <= 0) { /* Class is only used by a single public module. Put it there. * * Note that we must not do this if there are any dynamic or excluded @@ -387,6 +384,9 @@ private object Tagger { private final class DynamicPaths { private val content = mutable.Map.empty[ClassName, DynamicPaths] + /** + * @return `true` iff the end of the path changed. + */ @tailrec def put(path: List[ClassName]): Boolean = { val h :: t = path From f72c81a520bdc9c9faeebc01a2f4c85776dfed8b Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Wed, 22 May 2024 17:54:54 +1200 Subject: [PATCH 13/16] Update comment --- .../frontend/modulesplitter/Tagger.scala | 184 +++++++++--------- 1 file changed, 94 insertions(+), 90 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index 2aa704ab62..deec9a2d26 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -24,138 +24,139 @@ import scala.collection.{immutable, mutable} /** Tagger groups classes into coarse modules. * - * It is the primary mechanism for the FewestModulesAnalyzer but also used - * by the SmallModulesForAnalyzer. + * It is the primary mechanism for the FewestModulesAnalyzer but also used + * by the SmallModulesForAnalyzer. * - * To group classes into modules appropriately, we want to know for - * each class, "how" it can be reached. In practice, this means we - * record the path from the original public module and every - * dynamic import hop we made. + * To group classes into modules appropriately, we want to know for + * each class, "how" it can be reached. In practice, this means we + * record the path from the original public module and every + * dynamic import hop we made. * - * Of all these paths, we only care about the "simplest" ones. Or - * more formally, the minimum prefixes of all paths. For example, - * if a class is reachable by the following paths: + * Of all these paths, we only care about the "simplest" ones. Or + * more formally, the minimum prefixes of all paths. For example, + * if a class is reachable by the following paths: * * - a -> b * - a -> b -> c * - d -> c * - d * - * We really only care about: + * We really only care about: * * - a -> b * - d * - * Because if we reach the class through a path that goes through - * `c`, it is necessarily already loaded. + * Because if we reach the class through a path that goes through + * `c`, it is necessarily already loaded. * - * Once we have obtained this minimal set of paths, we use the last - * element of each path to determine the final module - * grouping. This is because these have an actual static dependency - * on the node in question. + * Once we have obtained this minimal set of paths, we use the last + * element of each path to determine the final module + * grouping. This is because these have an actual static dependency + * on the node in question. * - * Merging these tags into a single `ModuleID` is delegated to the - * caller. + * Merging these tags into a single `ModuleID` is delegated to the + * caller. * - * == Class Exclusion == - * Classes can be excluded from the modules generated by Tagger. + * == Class Exclusion == + * Classes can be excluded from the modules generated by Tagger. * - * For excluded classes, the Tagger assumes that they are in a module - * provided by a different part of the overall analysis. + * For excluded classes, the Tagger assumes that they are in a module + * provided by a different part of the overall analysis. * - * The (transitive) dependencies of the class are nevertheless taken into - * account and tagged as appropriate. - * In particular, to avoid cycles and excessive splitting alike (see #4835), - * we need to introduce an additional tagging mechanism. + * The (transitive) dependencies of the class are nevertheless taken into + * account and tagged as appropriate. + * In particular, to avoid cycles and excessive splitting alike (see #4835), + * we need to introduce an additional tagging mechanism. * - * To illustrate the problem, take the following dependency graph as an example + * To illustrate the problem, take the following dependency graph as an example * - * a -> b -> c + * a -> b -> c * - * where B is excluded. Naively, we would want to group a and c together into a'. - * However, this would lead to a circular dependency between a' and c. + * where B is excluded. Naively, we would want to group a and c together into a'. + * However, this would lead to a circular dependency between a' and c. * - * Nevertheless, in the absence of b, or if b is not an excluded class, we'd - * want to perform the grouping to avoid unnecessary splitting. + * Nevertheless, in the absence of b, or if b is not an excluded class, we'd + * want to perform the grouping to avoid unnecessary splitting. * - * We achieve this by tracking an additional tag, representing the maximum - * number of hops from an excluded class (aka fine) to a non-excluded class - * (aka coarse) class for any path from an entrypoint to the given class. + * We achieve this by tracking an additional tag, representing the maximum + * number of hops from an excluded class (aka fine) to a non-excluded class + * (aka coarse) class for any path from an entrypoint to the given class. * - * We then only permit grouping coarse classes with the same tag. This avoids - * the creation of cycles. + * We then only permit grouping coarse classes with the same tag. This avoids + * the creation of cycles. * - * The following is a proof that this strategy avoids cycles. + * The following is a proof that this strategy avoids cycles. * - * Given + * Given * - * G = (V, E), acyclic, V = F ∪ C, F ∩ C = ∅ - * the original dependency graph, - * F: set of fine classes, - * C: set of coarse classes + * G = (V, E), acyclic, V = F ∪ C, F ∩ C = ∅ + * the original dependency graph, + * F: set of fine classes, + * C: set of coarse classes * - * t : V → ℕ (the maxExcludedHopCount tag) - * ∀ (v1, v2) ∈ E : t(v1) ≤ t(v2) - * ∀ (f, c) ∈ E : f ∈ F, c ∈ C ⇒ t(f) < t(c) + * t : V → ℕ (the maxExcludedHopCount tag) + * ∀ (v1, v2) ∈ E : t(v1) ≤ t(v2) + * ∀ (f, c) ∈ E : f ∈ F, c ∈ C ⇒ t(f) < t(c) * - * Define + * Define * - * G' = (V', E'), V' = F ∪ C' (the new grouped graph) + * G' = (V', E'), V' = F ∪ C' (the new grouped graph) * - * C' = { n ∈ ℕ | ∃ c ∈ C : t(c) = n } + * C' = { n ∈ ℕ | ∃ c ∈ C : t(c) = n } * - * E' = { (f1, f2) ∈ E | f1, f2 ∈ F } ∪ - * { (f, n) | f ∈ F, ∃ c ∈ C : (f, c) ∈ E : t(c) = n } ∪ - * { (n, f) | f ∈ F, ∃ c ∈ C : (c, f) ∈ E : t(c) = n } ∪ - * { (n, m) | n ≠ m, ∃ c1, c2 ∈ C : (c1, c2) ∈ E : t(c1) = n, t(c2) = m } + * E' = { (f1, f2) ∈ E | f1, f2 ∈ F } ∪ + * { (f, n) | f ∈ F, ∃ c ∈ C : (f, c) ∈ E : t(c) = n } ∪ + * { (n, f) | f ∈ F, ∃ c ∈ C : (c, f) ∈ E : t(c) = n } ∪ + * { (n, m) | n ≠ m, ∃ c1, c2 ∈ C : (c1, c2) ∈ E : t(c1) = n, t(c2) = m } * - * t' : V' → ℕ: + * t' : V' → ℕ: * - * t'(f) = t(f) (if f ∈ F) - * t'(n) = n (if n ∈ C') + * t'(f) = t(f) (if f ∈ F) + * t'(n) = n (if n ∈ C') * - * Lemma 1 (unproven) + * Lemma 1 (unproven) * - * ∀ (v1, v2) ∈ E' : t'(v1) ≤ t'(v2) + * ∀ (v1, v2) ∈ E' : t'(v1) ≤ t'(v2) * - * Lemma 2 (unproven) + * Lemma 2 (unproven) * - * ∀ (f, n) ∈ E' : f ∈ F, n ∈ C' : t'(f) < t'(n) + * ∀ (f, n) ∈ E' : f ∈ F, n ∈ C' : t'(f) < t'(n) * - * Lemma 3 + * Lemma 3 * - * ∀ (n, m) ∈ E' : n,m ∈ C' ⇒ t'(n) < t'(m) + * ∀ (n, m) ∈ E' : n,m ∈ C' ⇒ t'(n) < t'(m) * - * Follows from Lemma 1 and (n, m) ∈ E' ⇒ n ≠ m (by definition). + * Follows from Lemma 1 and (n, m) ∈ E' ⇒ n ≠ m (by definition). * - * Theorem + * Theorem * - * G' is acyclic + * G' is acyclic * - * Proof by contradiction. + * Proof by contradiction. * - * Assume ∃ p = x1, ..., xn (x1 = xn, n > 1, xi ∈ V) + * Assume ∃ p = x1, ..., xn (x1 = xn, n > 1, xi ∈ V) * - * ∃ xi ∈ C' by contradiction: ∀ xi ∈ F ⇒ p is a cycle in G + * ∃ xi ∈ C' by contradiction: ∀ xi ∈ F ⇒ p is a cycle in G * - * ∃ xi ∈ F by contradiction: ∀ xi ∈ C' ⇒ - * t'(xi) increases strictly monotonically (by Lemma 3), - * but x1 = xn ⇒ t'(x1) = t'(xn) + * ∃ xi ∈ F by contradiction: ∀ xi ∈ C' ⇒ + * t'(xi) increases strictly monotonically (by Lemma 3), + * but x1 = xn ⇒ t'(x1) = t'(xn) * - * Therefore, + * Therefore, * - * ∃ (xi, xj) ∈ p : xi ∈ F, xj ∈ C' + * ∃ (xi, xj) ∈ p : xi ∈ F, xj ∈ C' * - * Therefore (by Lemma 1) + * Therefore (by Lemma 1) * - * t'(x1) ≤ ... ≤ t'(xi) < t'(xj) ≤ ... ≤ t'(xn) ⇒ t'(x1) < t'(xn) + * t'(x1) ≤ ... ≤ t'(xi) < t'(xj) ≤ ... ≤ t'(xn) ⇒ t'(x1) < t'(xn) * - * But x1 = xn ⇒ t'(x1) = t'(xn), which is a contradiction. + * But x1 = xn ⇒ t'(x1) = t'(xn), which is a contradiction. * - * Therefore, G' is acyclic. + * Therefore, G' is acyclic. */ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, - excludedClasses: scala.collection.Set[ClassName] = Set.empty) { + excludedClasses: scala.collection.Set[ClassName] = Set.empty) { + import Tagger._ private[this] val allPaths = mutable.Map.empty[ClassName, Paths] @@ -180,9 +181,9 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, * also re-tagged where appropriate. * * @param classNames the starting set of classes to tag - * @param pathRoot the root of the path - * @param pathSteps the steps that make up path - * @param nextSteps the accumulated result + * @param pathRoot the root of the path + * @param pathSteps the steps that make up path + * @param nextSteps the accumulated result * @return the next steps to traverse */ @tailrec @@ -192,8 +193,8 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, case Some(className) => allPaths.get(className) match { case None => val paths = new Paths(trackExcludedHopCounts = excludedClasses.nonEmpty) - paths.put(pathRoot, pathSteps) allPaths.update(className, paths) + paths.put(pathRoot, pathSteps) // Consider dependencies the first time we encounter them as this is the shortest path there will be. val classInfo = infos.classDependencies(className) tag(classNames.tail ++ classInfo.staticDependencies, pathRoot, pathSteps, @@ -202,10 +203,10 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, val moduleIDChanged = paths.put(pathRoot, pathSteps) val nextClassNames = if (moduleIDChanged) { - // TODO: What about dynamic dependencies? I think we need to revisit them too // Revisit static dependencies again when the module id changes. // We do not need to revisit dynamic dependencies because by definition they are not used by public - // modules, the first time we tag them is the shortest path so that path end never changes + // modules, the first time we tag them is the shortest path so that path end never changes and only the + // ends are used for the module ids. classNames.tail ++ infos.classDependencies(className).staticDependencies } else { // Otherwise do not consider dependencies again as there is no more information to find. @@ -224,9 +225,9 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, * dependencies have been tagged. * * @param classNames the steps to tag and traverse - * @param pathRoot the root of the path - * @param pathSteps the steps that make up path - * @param acc the accumulator + * @param pathRoot the root of the path + * @param pathSteps the steps that make up path + * @param acc the accumulator */ @tailrec private def tagNextSteps(classNames: Set[ClassName], pathRoot: ModuleID, pathSteps: List[ClassName], @@ -250,9 +251,9 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, * * This will traverse dependencies repeatedly if a prefix is found with a larger excluded hop count. * - * @param className the starting class to tag + * @param className the starting class to tag * @param excludedHopCount the excluded hop count so far - * @param fromExcluded whether the previous step was excluded + * @param fromExcluded whether the previous step was excluded */ private def updateExcludedHopCounts(className: ClassName, excludedHopCount: Int, fromExcluded: Boolean): Unit = { val isExcluded = excludedClasses.contains(className) @@ -294,7 +295,7 @@ private object Tagger { /** "Interesting" paths that can lead to a given class. * - * "Interesting" in this context means: + * "Interesting" in this context means: * - All direct paths from a public dependency. * - All non-empty, mutually prefix-free paths of dynamic import hops. */ @@ -317,6 +318,9 @@ private object Tagger { } } + /** + * @return `true` if the moduleID for this class changed + */ def updateExcludedHopCount(excludedHopCount: Int): Boolean = { val hopCountsChanged = excludedHopCount > maxExcludedHopCount if (hopCountsChanged) From e47f9d57881defea54da21e317f42f719a43aaca Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Thu, 23 May 2024 14:35:54 +1200 Subject: [PATCH 14/16] Add tests --- .../frontend/modulesplitter/TaggerTest.scala | 434 ++++++++++++++++++ 1 file changed, 434 insertions(+) create mode 100644 linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala diff --git a/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala new file mode 100644 index 0000000000..8572b00ec2 --- /dev/null +++ b/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala @@ -0,0 +1,434 @@ +/* + * Scala.js (https://www.scala-js.org/) + * + * Copyright EPFL. + * + * Licensed under Apache License 2.0 + * (https://www.apache.org/licenses/LICENSE-2.0). + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +package org.scalajs.linker.frontend.modulesplitter + +import org.junit.Assert._ +import org.junit.Test +import org.scalajs.ir.Names.ClassName +import org.scalajs.linker.frontend.modulesplitter.ModuleAnalyzer.{ClassInfo, DependencyInfo} +import org.scalajs.linker.standard.ModuleSet.ModuleID + +import scala.collection.immutable.{ListMap, ListSet} + +class TaggerTest { + + @Test def testModuleIDOfStaticUnchangedWithNewDirectPathSameModule(): Unit = { + // Initially the class has one direct path. + // Ensure that adding a direct path from the same public modules will not change the module ID. + + val moduleAID = ModuleID("modA") + + val publicAClassName = ClassName("publicA") + val publicBClassName = ClassName("publicB") + val staticClassName = ClassName("static") + + val classDependencies = ListMap( + publicAClassName -> new ClassInfo(ListSet(staticClassName), Set.empty), + publicBClassName -> new ClassInfo(ListSet(staticClassName), Set.empty), + staticClassName -> new ClassInfo(Set.empty, Set.empty) + ) + + val initialModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicAClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val finalModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicAClassName, publicBClassName), + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val initialStaticModuleID = initialModuleMap(staticClassName) + + val finalPublicModuleID = finalModuleMap(publicAClassName) + val finalStaticModuleID = finalModuleMap(staticClassName) + + assertEquals(initialStaticModuleID, finalStaticModuleID) + assertEquals(finalPublicModuleID, finalStaticModuleID) + } + + @Test def testModuleIDOfStaticChangesWithNewDirectPathNewModule(): Unit = { + // Initially the class has one direct path. + // Ensure that adding a direct paths from a new public module will change the module ID. + + val moduleAID = ModuleID("modA") + val moduleBID = ModuleID("modB") + + val publicClassName = ClassName("public") + val staticClassName = ClassName("static") + + val classDependencies = ListMap( + publicClassName -> new ClassInfo(ListSet(staticClassName), Set.empty), + staticClassName -> new ClassInfo(Set.empty, Set.empty) + ) + + val initialModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val finalModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName), + moduleBID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val initialStaticModuleID = initialModuleMap(staticClassName) + + val finalStaticModuleID = finalModuleMap(staticClassName) + val finalPublicModuleID = finalModuleMap(publicClassName) + + assertNotEquals(initialStaticModuleID, finalStaticModuleID) + assertEquals(finalPublicModuleID, finalStaticModuleID) + } + + @Test def testModuleIDOfStaticChangesWithNewTransitiveDynamicPathSameModule(): Unit = { + // Initially the class has one direct path. + // Ensure that adding a dynamic path from the same public modules will change the module ID. + + val moduleAID = ModuleID("modA") + + val publicClassName = ClassName("public") + val dynamicClassName = ClassName("dynamic") + val staticClassName = ClassName("static") + + val initialModuleMap = locally { + val classDependencies = ListMap( + publicClassName -> new ClassInfo(ListSet(staticClassName), Set.empty), + staticClassName -> new ClassInfo(Set.empty, Set.empty) + ) + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val finalModuleMap = locally { + val classDependencies = ListMap( + publicClassName -> new ClassInfo(ListSet(staticClassName), ListSet(dynamicClassName)), + dynamicClassName -> new ClassInfo(ListSet(staticClassName), Set.empty), + staticClassName -> new ClassInfo(Set.empty, Set.empty) + ) + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val initialStaticModuleID = initialModuleMap(staticClassName) + + val finalPublicModuleID = finalModuleMap(publicClassName) + val finalDynamicModuleID = finalModuleMap(dynamicClassName) + val finalStaticModuleID = finalModuleMap(staticClassName) + + assertNotEquals(initialStaticModuleID, finalStaticModuleID) + assertNotEquals(finalPublicModuleID, finalStaticModuleID) + // TODO: Why are there 3 modules and not 2? + assertNotEquals(finalDynamicModuleID, finalStaticModuleID) + } + + @Test def testModuleIDOfStaticChangesWithNewDynamicPathSameModule(): Unit = { + // Initially the class has one direct path. + // Ensure that adding a dynamic path from the same public modules will change the module ID. + + val moduleAID = ModuleID("modA") + + val publicClassName = ClassName("public") + val dynamicClassName = ClassName("dynamic") + val staticClassName = ClassName("static") + + val initialModuleMap = locally { + val classDependencies = ListMap( + publicClassName -> new ClassInfo(ListSet(staticClassName), Set.empty), + staticClassName -> new ClassInfo(Set.empty, Set.empty) + ) + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val finalModuleMap = locally { + val classDependencies = ListMap( + publicClassName -> new ClassInfo(ListSet(staticClassName), ListSet(dynamicClassName)), + dynamicClassName -> new ClassInfo(Set.empty, ListSet(staticClassName)), + staticClassName -> new ClassInfo(Set.empty, Set.empty) + ) + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val initialStaticModuleID = initialModuleMap(staticClassName) + + val finalPublicModuleID = finalModuleMap(publicClassName) + val finalDynamicModuleID = finalModuleMap(dynamicClassName) + val finalStaticModuleID = finalModuleMap(staticClassName) + + assertNotEquals(initialStaticModuleID, finalStaticModuleID) + assertNotEquals(finalPublicModuleID, finalStaticModuleID) + // TODO: Why are there 3 modules and not 2? + assertNotEquals(finalDynamicModuleID, finalStaticModuleID) + } + + @Test def testModuleIDOfDynamicChangesWithNewDirectPathSameModule(): Unit = { + // Initially the class has one dynamic path. + // Ensure that adding a direct path from the same public modules will change the module ID (the first time only). + + val moduleAID = ModuleID("modA") + + val publicAClassName = ClassName("publicA") + val publicBClassName = ClassName("publicB") + val publicCClassName = ClassName("publicC") + val dynamicClassName = ClassName("dynamic") + + val classDependencies = ListMap( + publicAClassName -> new ClassInfo(Set.empty, ListSet(dynamicClassName)), + publicBClassName -> new ClassInfo(ListSet(dynamicClassName), Set.empty), + publicCClassName -> new ClassInfo(ListSet(dynamicClassName), Set.empty), + dynamicClassName -> new ClassInfo(Set.empty, Set.empty) + ) + + val initialModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicAClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val midModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicAClassName, publicBClassName), + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val finalModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicAClassName, publicBClassName, publicCClassName), + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val initialDynamicModuleID = initialModuleMap(dynamicClassName) + + val midPublicModuleID = midModuleMap(publicAClassName) + val midDynamicModuleID = midModuleMap(dynamicClassName) + + val finalDynamicModuleID = finalModuleMap(dynamicClassName) + + assertNotEquals(initialDynamicModuleID, midDynamicModuleID) + assertEquals(midDynamicModuleID, finalDynamicModuleID) + assertNotEquals(midPublicModuleID, midDynamicModuleID) + } + + @Test def testModuleIDOfDynamicChangesWithNewDirectPathNewModule(): Unit = { + // Initially the class has one dynamic path. + // Ensure that adding a direct paths from a new public module will change the module ID. + + val moduleAID = ModuleID("modA") + val moduleBID = ModuleID("modB") + + val publicAClassName = ClassName("publicA") + val publicBClassName = ClassName("publicB") + val dynamicClassName = ClassName("dynamic") + + val classDependencies = ListMap( + publicAClassName -> new ClassInfo(Set.empty, ListSet(dynamicClassName)), + publicBClassName -> new ClassInfo(ListSet(dynamicClassName), Set.empty), + dynamicClassName -> new ClassInfo(Set.empty, Set.empty) + ) + + val initialModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicAClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val finalModuleMap = locally { + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicAClassName), + moduleBID -> ListSet(publicBClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val initialDynamicModuleID = initialModuleMap(dynamicClassName) + + val finalDynamicModuleID = finalModuleMap(dynamicClassName) + val finalPublicModuleID = finalModuleMap(publicAClassName) + + assertNotEquals(initialDynamicModuleID, finalDynamicModuleID) + assertNotEquals(finalPublicModuleID, finalDynamicModuleID) + } + + @Test def testModuleIDOfDynamicChangesWithNewTransitiveDynamicPathSameModule(): Unit = { + // Initially the class has one dynamic path. + // Ensure that adding a dynamic path from the same public modules will change the module ID. + + val moduleAID = ModuleID("modA") + + val publicClassName = ClassName("public") + val dynamicAClassName = ClassName("dynamicA") + val dynamicBClassName = ClassName("dynamicB") + + val initialModuleMap = locally { + val classDependencies = ListMap( + publicClassName -> new ClassInfo(Set.empty, ListSet(dynamicBClassName)), + dynamicBClassName -> new ClassInfo(Set.empty, Set.empty) + ) + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val finalModuleMap = locally { + val classDependencies = ListMap( + publicClassName -> new ClassInfo(Set.empty, ListSet(dynamicAClassName, dynamicBClassName)), + dynamicAClassName -> new ClassInfo(ListSet(dynamicBClassName), Set.empty), + dynamicBClassName -> new ClassInfo(Set.empty, Set.empty) + ) + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val initialDynamicBModuleID = initialModuleMap(dynamicBClassName) + + val finalPublicModuleID = finalModuleMap(publicClassName) + val finalDynamicAModuleID = finalModuleMap(dynamicAClassName) + val finalDynamicBModuleID = finalModuleMap(dynamicBClassName) + + assertNotEquals(initialDynamicBModuleID, finalDynamicBModuleID) + assertNotEquals(finalPublicModuleID, finalDynamicBModuleID) + // TODO: Why are there 3 modules and not 2? + assertNotEquals(finalDynamicAModuleID, finalDynamicBModuleID) + } + + @Test def testModuleIDOfDynamicUnchangedWithNewDynamicPathSameModule(): Unit = { + // Initially the class has one dynamic path. + // Ensure that adding a dynamic path from the same public modules will not change the module ID. + // This does not change because a non-transitive dynamic only has itself as the path end. + + val moduleAID = ModuleID("modA") + + val publicClassName = ClassName("public") + val dynamicAClassName = ClassName("dynamicA") + val dynamicBClassName = ClassName("dynamicB") + + val initialModuleMap = locally { + val classDependencies = ListMap( + publicClassName -> new ClassInfo(Set.empty, ListSet(dynamicBClassName)), + dynamicBClassName -> new ClassInfo(Set.empty, Set.empty) + ) + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val finalModuleMap = locally { + val classDependencies = ListMap( + publicClassName -> new ClassInfo(Set.empty, ListSet(dynamicAClassName, dynamicBClassName)), + dynamicAClassName -> new ClassInfo(Set.empty, ListSet(dynamicBClassName)), + dynamicBClassName -> new ClassInfo(Set.empty, Set.empty) + ) + val publicModuleDependencies = ListMap( + moduleAID -> ListSet(publicClassName) + ) + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + val tagger = new Tagger(info, Set.empty) + tagger.tagAll(Iterable.empty) + } + + val initialDynamicBModuleID = initialModuleMap(dynamicBClassName) + + val finalPublicModuleID = finalModuleMap(publicClassName) + val finalDynamicAModuleID = finalModuleMap(dynamicAClassName) + val finalDynamicBModuleID = finalModuleMap(dynamicBClassName) + + assertEquals(initialDynamicBModuleID, finalDynamicBModuleID) + assertNotEquals(finalPublicModuleID, finalDynamicBModuleID) + // TODO: Why are there 3 modules and not 2? + assertNotEquals(finalDynamicAModuleID, finalDynamicBModuleID) + } + + @Test(timeout = 1000) def testLinkedDynamics(): Unit = { + val mainModuleID = ModuleID("main") + + val publicClassName = ClassName("public") + + val routeClassNames = (1 to 100).map(n => ClassName(s"route$n")).toSet + + val classDependencies = ListMap( + publicClassName -> new ClassInfo(Set.empty, routeClassNames) + ) ++ routeClassNames.map(_ -> new ClassInfo(Set.empty, routeClassNames)) + + val publicModuleDependencies = ListMap(mainModuleID -> ListSet(publicClassName)) + + val info = new DependencyInfo(classDependencies, publicModuleDependencies) + + val excludedClasses = Set.empty[ClassName] + + val tagger = new Tagger(info, excludedClasses) + + val moduleMap = tagger.tagAll(Iterable.empty) + + val subjectModuleID = moduleMap(publicClassName) + assertSame(mainModuleID, subjectModuleID) + } +} From 92d63b58f995b712f11bbf4ff3555b9931d02f41 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Thu, 23 May 2024 14:38:40 +1200 Subject: [PATCH 15/16] Update comment --- .../org/scalajs/linker/frontend/modulesplitter/Tagger.scala | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala index deec9a2d26..bae92faf41 100644 --- a/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala +++ b/linker/shared/src/main/scala/org/scalajs/linker/frontend/modulesplitter/Tagger.scala @@ -204,9 +204,8 @@ private class Tagger(infos: ModuleAnalyzer.DependencyInfo, val nextClassNames = if (moduleIDChanged) { // Revisit static dependencies again when the module id changes. - // We do not need to revisit dynamic dependencies because by definition they are not used by public - // modules, the first time we tag them is the shortest path so that path end never changes and only the - // ends are used for the module ids. + // We do not need to revisit dynamic dependencies because their module id only changes when there is a + // new usage which is the case above and there we already revisit them. classNames.tail ++ infos.classDependencies(className).staticDependencies } else { // Otherwise do not consider dependencies again as there is no more information to find. From bf0d1595f32fb915504bee809ef6a92e3e8a2933 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Wed, 5 Jun 2024 17:52:31 +1200 Subject: [PATCH 16/16] Remove trailing commas --- .../scalajs/linker/frontend/modulesplitter/TaggerTest.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala index 8572b00ec2..e7c17d51ab 100644 --- a/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala +++ b/linker/shared/src/test/scala/org/scalajs/linker/frontend/modulesplitter/TaggerTest.scala @@ -49,7 +49,7 @@ class TaggerTest { val finalModuleMap = locally { val publicModuleDependencies = ListMap( - moduleAID -> ListSet(publicAClassName, publicBClassName), + moduleAID -> ListSet(publicAClassName, publicBClassName) ) val info = new DependencyInfo(classDependencies, publicModuleDependencies) val tagger = new Tagger(info, Set.empty) @@ -235,7 +235,7 @@ class TaggerTest { val midModuleMap = locally { val publicModuleDependencies = ListMap( - moduleAID -> ListSet(publicAClassName, publicBClassName), + moduleAID -> ListSet(publicAClassName, publicBClassName) ) val info = new DependencyInfo(classDependencies, publicModuleDependencies) val tagger = new Tagger(info, Set.empty) @@ -244,7 +244,7 @@ class TaggerTest { val finalModuleMap = locally { val publicModuleDependencies = ListMap( - moduleAID -> ListSet(publicAClassName, publicBClassName, publicCClassName), + moduleAID -> ListSet(publicAClassName, publicBClassName, publicCClassName) ) val info = new DependencyInfo(classDependencies, publicModuleDependencies) val tagger = new Tagger(info, Set.empty)