Consolidate Info Loading #4824

gzm0 · 2023-03-05T19:52:17Z

Instead of individual loaders for BaseLinker and Refiner, we
consolidate into a single loader.

gzm0 · 2023-03-05T19:52:56Z

Note that this currently fails org.scalajs.linker.BasicLinkerBackendTest.linkNoSecondAttemptInEmitter I wanted to get an opinion first, before debugging that.

gzm0 · 2023-03-05T19:54:07Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/IRLoader.scala

-
-private object ClassDefAndInfoCache {
-  final class Update(
-      val classDef: ClassDef,


Seems we currently cache the entire ClassDef, even though the global IRCache already does that.

sjrd

That seems appropriate. It's overall a nice simplification, I think. The added caching probably won't do any good, but it also should not hurt. It might become beneficial with an incremental Analyzer?

linker/shared/src/main/scala/org/scalajs/linker/frontend/Refiner.scala

linker/shared/src/main/scala/org/scalajs/linker/frontend/IRLoader.scala

sjrd · 2023-03-13T19:44:24Z

linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/KnowledgeGuardian.scala

@@ -233,27 +233,16 @@ private[emitter] final class KnowledgeGuardian(config: Emitter.Config) {

    private var isAlive: Boolean = true

-    private var isInterface = computeIsInterface(initClass)
+    private var linkedClass = initClass


Typo in the commit message? "Doesn't require use to compare"

sjrd · 2023-03-13T19:54:52Z

linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/KnowledgeGuardian.scala

-      superClassAskers += invalidatable
-      superClass
+      rawClassInfoAskers += invalidatable
+      linkedClass.superClass.fold[ClassName](null.asInstanceOf[ClassName])(_.name)


Is asInstanceOf necessary, here?

Suggested change

linkedClass.superClass.fold[ClassName](null.asInstanceOf[ClassName])(_.name)

linkedClass.superClass.fold[ClassName](null)(_.name)

sjrd · 2023-03-13T19:57:59Z

linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/KnowledgeGuardian.scala

+      rawClassInfoAskers += invalidatable
+      linkedClass.fields


Is this valid? Can't fields evolve independently of linkedClass.version because of reachability analysis?

Uh, oh. Indeed :-/

sjrd · 2023-03-13T20:00:23Z

linker/js/src/main/scala/org/scalajs/linker/backend/emitter/PrivateLibHolder.scala

+      val tree = ir.Serializers.deserialize(ByteBuffer.wrap(content))
+
+      new IRFileImpl(path, stableVersion) {
+        def entryPointsInfo(implicit ec: ExecutionContext): Future[ir.EntryPointsInfo] =


CI says: missing import for ExecutionContext and Future.

sjrd · 2023-03-13T20:02:39Z

linker/js/src/main/scala/org/scalajs/linker/backend/emitter/PrivateLibHolder.scala

-      )
+      val path = "org/scalajs/linker/runtime/" + name
+      val content = Base64.getDecoder().decode(contentBase64)
+      val tree = ir.Serializers.deserialize(ByteBuffer.wrap(content))


Why don't we do the same thing in linker/jvm/.../PrivateLibHolder?

Because I forgot :-/

sjrd · 2023-03-13T20:11:39Z

linker/shared/src/main/scala/org/scalajs/linker/analyzer/InfoLoader.scala

+  case object InternalIRCheck extends IRCheckMode
+}
+
+private class ClassInfoCache(className: ClassName, irLoader: IRLoader, irCheckMode: InfoLoader.IRCheckMode) {


Consider putting all these private classes inside object InfoLoader. We usually do that.

sjrd · 2023-03-13T20:12:56Z

linker/shared/src/main/scala/org/scalajs/linker/analyzer/InfoLoader.scala

+      case FieldDef(flags, FieldIdent(name), _, ftpe) =>
+          if (!flags.namespace.isStatic)
+            builder.maybeAddReferencedFieldClass(name, ftpe)


Indentation issue.

sjrd · 2023-03-13T22:50:18Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/BaseLinker.scala

  private val methodSynthesizer = new MethodSynthesizer(irLoader)
+  private val infoLoader = new InfoLoader(irLoader,


Looks like this right hand side deserves a block.

sjrd · 2023-03-13T22:55:23Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/BaseLinker.scala

@@ -107,11 +115,11 @@ final class BaseLinker(config: CommonPhaseConfig, checkIR: Boolean) {
  private def assemble(moduleInitializers: Seq[ModuleInitializer],
      analysis: Analysis)(implicit ec: ExecutionContext): Future[LinkingUnit] = {
    def assembleClass(info: ClassInfo) = {
-      val classAndVersion = irLoader.loadClassDefAndVersion(info.className)
+      val version = irLoader.irFileVersion(info.className)


This split between the version and the tree does not seem warranted. It results in looking up the class name twice in a hash map instead of once. Why is this done?

To avoid triggering a load if the version stays the same (in other parts of the codebase):

Alternatives:

(re)-add an additional method loadClassDefAndVersion to IRLoader. Con: More complicated interface.

Return IRFiles from IRLoader (or Option[IRFile]). Con: Needs wrapping in the Refiner (OTOH, we could replace the tuples returned by the Optimizer by an IRFile subclass).

Ah yes, I see. I wouldn't go as far as returning IRFile from IRLoader. I think I would find that confusing. Either re-add loadClassDefAndVersion or leave it as is seem equally good.

sjrd · 2023-03-13T23:00:14Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/Refiner.scala


 /** Does a dead code elimination pass on a [[LinkingUnit]]. */
 final class Refiner(config: CommonPhaseConfig, checkIR: Boolean) {
  import Refiner._

-  private val inputProvider = new InputProvider(checkIR)
+  private val irLoader = new ClassDefIRLoader
+  private val infoLoader = new InfoLoader(irLoader,


Same as the other one. This deserves a block.

gzm0 · 2023-04-09T15:59:43Z

Rebased onto main and addressed comments.

TBC: Not necessary for 1.13.1.

sjrd

LGTM module the suggested Squash.

Instead of individual loaders for BaseLinker and Refiner, we consolidate into a single loader.

gzm0 commented Mar 5, 2023

View reviewed changes

sjrd reviewed Mar 6, 2023

View reviewed changes

linker/shared/src/main/scala/org/scalajs/linker/frontend/Refiner.scala Show resolved Hide resolved

linker/shared/src/main/scala/org/scalajs/linker/frontend/IRLoader.scala Show resolved Hide resolved

This was referenced Mar 12, 2023

Emitter#FullClassCache#getOrElseUpdate relies on identity of ir trees (not js trees) #4830

Closed

emitter.KnowledgeGuardian incorrectly compares field defs #4831

Closed

gzm0 force-pushed the info-loader branch from 47f8b0f to 9bab761 Compare March 12, 2023 15:51

gzm0 requested a review from sjrd March 12, 2023 16:21

gzm0 marked this pull request as ready for review March 12, 2023 16:21

gzm0 changed the title ~~RFC: Consolidate Info Loading~~ Consolidate Info Loading Mar 12, 2023

sjrd requested changes Mar 13, 2023

View reviewed changes

sjrd mentioned this pull request Mar 25, 2023

Eagerly deserialize linker private lib #4839

Merged

gzm0 force-pushed the info-loader branch from 9bab761 to b892c04 Compare April 9, 2023 15:58

gzm0 requested a review from sjrd April 9, 2023 15:59

gzm0 mentioned this pull request Apr 9, 2023

Keep the analyzer alive over incremental runs #4832

Merged

Use a single globalIRCache for testing

3a50066

gzm0 force-pushed the info-loader branch from 857fd80 to 86f4924 Compare April 10, 2023 13:34

sjrd approved these changes Apr 10, 2023

View reviewed changes

Consolidate Info Loading

ae17b93

Instead of individual loaders for BaseLinker and Refiner, we consolidate into a single loader.

gzm0 force-pushed the info-loader branch from 86f4924 to ae17b93 Compare April 10, 2023 14:28

gzm0 merged commit 9781655 into scala-js:main Apr 11, 2023
3 checks passed

gzm0 deleted the info-loader branch April 16, 2023 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate Info Loading #4824

Consolidate Info Loading #4824

gzm0 commented Mar 5, 2023 •

edited

gzm0 commented Mar 5, 2023

gzm0 Mar 5, 2023

sjrd left a comment

sjrd Mar 13, 2023

sjrd Mar 13, 2023

sjrd Mar 13, 2023

gzm0 Mar 15, 2023

sjrd Mar 13, 2023

sjrd Mar 13, 2023

gzm0 Mar 15, 2023

sjrd Mar 13, 2023

sjrd Mar 13, 2023

sjrd Mar 13, 2023

sjrd Mar 13, 2023

gzm0 Mar 15, 2023

sjrd Mar 15, 2023

sjrd Mar 13, 2023

gzm0 commented Apr 9, 2023

sjrd left a comment

	linkedClass.superClass.fold[ClassName](null.asInstanceOf[ClassName])(_.name)
	linkedClass.superClass.fold[ClassName](null)(_.name)

		private val methodSynthesizer = new MethodSynthesizer(irLoader)
		private val infoLoader = new InfoLoader(irLoader,

Consolidate Info Loading #4824

Consolidate Info Loading #4824

Conversation

gzm0 commented Mar 5, 2023 • edited

gzm0 commented Mar 5, 2023

Choose a reason for hiding this comment

sjrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 commented Apr 9, 2023

sjrd left a comment

Choose a reason for hiding this comment

gzm0 commented Mar 5, 2023 •

edited