Prototype v2 for Module Splitting

[gzm0]

Since the previous PR (#4010), the following has improved:

• The overall architecture is ready for review.
• The test suite passes in CommonJS.

[sjrd]

Some comments on the first commit. I don't have more time to continue this now, unfortunately, and I'll be on the road for the next 4 days. I'll come back to it next week.

[sjrd]

Suggested change

	val exportedMembers: List[ReachabilityInfo],
	val exportedMembers: List[ReachabilityInfo]

[sjrd]

What's the point of returning classDef? Shouldn't this function only return (classInfo, topLevelExportInfos)?

[sjrd]

Going further, I think generateClassInfo should stick with returning classInfo only, and generateTopLevelExportInfos should take a ClassDef (and actually be intended to be called publicly from elsewhere).

[sjrd]

Suggested change

	coreJSLibTrackedGlobalRefs ++ topLevelExportsTrackedGlobalRefs)(
	unionPreserveEmpty(coreJSLibTrackedGlobalRefs, topLevelExportsTrackedGlobalRefs))(

Unless we're using GCC, there is a high chance that both of these sets are empty.

[sjrd]

I don't think we can do that. If a class has only TopLevelMethodExportDefs, and is not otherwise needed, it will not appear at all in the unit.classDefs, and therefore lookupClass will not find it.

[gzm0]

Is this adaptation sufficiently backwards compatible? (it is not 100%).

[gzm0]

I have also considered to make a ModuleSpec a list inherently (like in an earlier proposal). However, it seems that with the fully automatic module tracking, the input ModuleSpec is unlikely to turn into something more automatic.

[gzm0]

I'm unsure where to put jsName / sourceMapName. If I put it here, it is nice for the sbt plugin, since it keeps full control. OTOH, we need more machinery in the sbt plugin to support the "mjs" suffix for node.

Maybe putting these into the config (and then maybe just make them suffix strings due to serializability) and instead return a LinkingReport containing the files that were written might be a better idea.

[sjrd]

AFAICT, it makes more sense for those things to be here. At least that way StandardConfig doesn't have to know about files.

[gzm0]

I'm less and less convinced this is the right spot TBH. I think we should have patterns for the following:

• JS file name
• Source map name
• Source map URI
• JS file URI
• module name (for include)

At that point, it really doesn't feel like this belongs to OutputSpec, but rather in a ModulePatterns option class in StandardConfig. WDYT?

[gzm0]

Hmmm... Turns out like this we cannot easily provide a backwards compatible method in Linker (because config cannot be change at that point).

I'm unsure how to proceed. IMHO as soon as things modify the contents of the files, it definitely should go into config :-/

[sjrd]

Perhaps one way to answer those questions: StandardConfig is just one way of configuring a linker, i.e., a standard linker. Does it make sense for another linker to be configured in a different way for file names? Would the sbt plugin be able to survive a different scalaJSLinker implementation that used a completely different scheme for file name configuration?

Today, the sbt plugin considers scalaJSLinkerConfig almost entirely as a blackbox, only used to create the default scalaJSLinkerConfig. The "almost" is because we actually dig into its moduleKind field to build jsEnv (and having some weird warning that if moduleKind differs in fast/fullOptJS than just in the configuration, things will break). To me this suggests that moduleKind already has a very specific status, and maybe shouldn't have been part of the scalaJSLinkerConfig to begin with. If these module patterns would equally affect the rest of the sbt plugin, perhaps they should stay out of StandardConfig?

[gzm0]

Hah, I think this is an excellent way of looking at it.

So what we observe is that the sbt plugin is peeking at the scalaJSLinkerConfig to find out which moduleKind the output is. It does this without taking the scalaJSStage into account, requiring the weird warning.

IMHO a better way to deal with this is a LinkingReport providing this information. At this point, the sbt plugin does not need to know/care anymore why a certain module type was returned (we could imagine a custom linker that only supports a specific type). It simply knows which module type was returned.

A similar strategy can then be applied to output files: The LinkingReport will simply list all the file names (and source maps) that have been produced.

This partially also addresses the backwards compatibility issue: We need not care about the config for file names but can

• write to a in memory directory
• retrieve the single output from the LinkingReport and write to the actual output

It does not address how to deal with sourceMapURI / jsFileURI.

[gzm0]

How about something like this?

final class OutputPatterns private (
    val jsFile: String,
    val sourceMapFile: String,
    val moduleName: String,
    val jsFileURI: String,
    val sourceMapURI: String
) {
  def this() = {
    this(
        jsFile = "[module-id].js",
        smFile = "[js-file].map",
        moduleName = "./[js-file]",
        jsFileURI = "[js-file]",
        smFileURI = "[sm-file]"
    )
  }

  // snip
}

This is inspired by webpack's placeholders. We could make this fully backwards compatible except:

• There would always be a link between source-map / js file.
• jsFileURI and sourceMapURI of a given LinkerOutput are set to the same (non-empty) value.

We'd have to combine this with a LinkingReport such that the sbt-plugin can recover root files and types.

@sjrd WDYT?

[gzm0]

Any idea what is happening here?

[gzm0]

Unsure what to do here. Should an empty module cause a failure? Should it cause an empty file? Should it cause no file?

[sjrd]

It seems to me that the most reasonable outcome would be an empty file. If a module has been requested by the user, and it ends up being empty, why should we get rid of it or consider that an error?

[gzm0]

why should we get rid of it or consider that an error?

It might hint at a configuration error. But yes, probably an empty module is OK.

[gzm0]

An alternative is to evolve the LinkingUnit into this class instead. What is unclear is how the Refiner would deal with a non-single module LinkingUnit.

[sjrd]

No, I think what you have now is better. It's better if front-end phases (the optimizer, but also future custom front-end phases) don't have to worry about modules.

[gzm0]

sbt plugin needs a lot more work. It's not 100% clear to me ATM how we can reasonably support existing builds that assume single-file outputs without introducing a lot of double logic.

[gzm0]

Rebased on top of #4120 and addressed the comments in the first commit (@sjrd WDYT about merging that one on its own?).

[sjrd]

Some comments as I finally get to review this, piece by piece.

Some things I've seen so far and that I like:

• The ModuleSpec concept.
• ModuleSplitStyle.
• The ModuleSet and ModuleSplitter concepts.

There's still a lot that I need to go through, so the above list is not yet exhaustive ;)

[sjrd]

Suggested change

	// Entry points (top-level exports and static initializers)
	// Static initializers

[sjrd]

This val is unused.

[sjrd]

Coding style nit:

Suggested change

	cacheUpdate = irFile.tree.map(tree =>
	(tree, Infos.generateClassInfo(tree), Infos.generateTopLevelExportInfos(tree)))
	cacheUpdate = irFile.tree.map { tree =>
	(tree, Infos.generateClassInfo(tree), Infos.generateTopLevelExportInfos(tree))
	}

[sjrd]

Suggested change

	val tree: TopLevelExportDef,
	val tree: TopLevelExportDef

[sjrd]

AFAICT, it makes more sense for those things to be here. At least that way StandardConfig doesn't have to know about files.

[sjrd]

Have you considered merging both of these in a single function?

val jsNameWithSourceMap: ModuleSpec.ModuleID => (String, String)

The name is uglier, but those things should always stick together, shouldn't they?

[sjrd]

Since directory is a mandatory item provided in apply, it probably does not make sense to add a withDirectory. I think so far we've not provided withX methods for x'es that are mandatory arguments of the apply method of config classes.

[sjrd]

Suggested change

	/** Convencience method to open a new channel this file's output directory. */
	/** Convenience method to open a new channel in this file's output directory. */

?

[sjrd]

Suggested change

	/** Convencience method to write this file in its output directory. */
	/** Convenience method to write this file in its output directory. */

[sjrd]

The raw info shouldn't be exposed in the Analysis. One use case of an Analysis is to build a separate tool that would render graphically/interactively the reachability graph of a Scala.js codebase, so it is a bit more public in spirit than Infos, which is really internal to the Analyzer.

[gzm0]

Hmmm... But what is the alternative? We need an info-like thing to determine module dependencies. Are you suggesting we re-calculate this if necessary with another tree traverse?

[sjrd]

We can potentially extend the Analysis, but now I see that there is a bigger problem: LinkedClass, which is in standard, contains references to Infos as well. That goes beyond intention, and directly into stabler-references-less-stable territory, which is not good at all.

From what I see, those Infos are needed in ModuleAnalyzer.staticDependencies. IIUC, this computes a dependencies: Set[ClassName]. Could we compute these dependencies eagerly in the Infos/Analyzer? Then we could expose that dependencies in Analysis (where it would make sense) and further in LinkedClass (which would therefore not depend on a type less stable than itself).

[gzm0]

Could we compute these dependencies eagerly in the Infos/Analyzer?

Yes. The first prototype did exactly that. Downsides:

• It makes the Analyzer even more complicated.
• The Analyzer now contains specifics to modules.
• We calculate it twice but only need it once (if we do not add yet another flag to the Analyzer).

I feel like the dependencies we calculate in ModuleAnalyzer are much more private than the Infos. Notably, they are not independent of other classes and depend on symbolRequirements. The Infos on the other hand are a mere summary of the trees of a class.

An alternative to what we have right now, might be to make Info caching more central. As such, both the Refiner and the ModuleAnalyzer could access Infos (without exposing them in the LinkedClass). It is unclear how to properly identify the LinkedClass in this case though.

[sjrd]

If we break down the things that staticDependencies computes, there are:

• Things read from ReachabilityInfo, among which
  • instantiatedClasses and accessedModules, which depend on isNonNative. Why do we need to know whether it is non-native? Could we get away without this knowledge? If not, we could store them in a separate set that the ModuleAnalyzer will filter for non-native classes.
  • Everything else, which do not depend on other classes.
• Things read directly from the classInfos, which can also be read from existing fields of LinkedClass, AFAICT
• Things depending on the symbolRequirements, which are only added to j.l.Object, and that the ModuleInitializer could patch up when reading the dependencies of j.l.Object (after all, that is clearly a private implementation detail of the ModuleInitializer).
• The artificial dependency from everything on j.l.Object, which the ModuleInitializer can also patch up when reading the dependencies from the LinkedClass.

That would remove all the dependencies on other classes and on symbolRequirements.

The Infos might be less private in terms of exposed information, but as an API they are very unstable, and clearly an implementation detail of the Analyzer.

[gzm0]

Why do we need to know whether it is non-native?

Because instantiating a native class does not actually need the SJS module that class is in. But the native module that class is in.

This is why we need the distinction between, for example, instantiatedClasses and methodsCalledStatically.keys. A static method of a native class is still an SJS thing. However, a new instance is not.

Could we get away without this knowledge?

No.

So in terms of ReachabilityInfo this establishes that we need finer granularity than simply a Set[ClassName]. The alternative I see here is that we make a difference between a static and an external dependency in the Analyzer. This is not really a breach of abstraction, since it follows from the IR spec (the distinction, what exactly constitutes a static dependency is, AFAICT an implementation detail of the Emitter, so we probably have a breach of abstraction either way).

The upside of this would be:

• No exposed infos.
• No exposed isParentDataAccessed

The downside of this would be:

• More complexity in the Analyzer.
• static and external dependencies are calculated pre optimizer for no reason (if it is enabled).
• static and external dependencies are calculated even without module splitting.

[gzm0]

@sjrd would you like me to try this? How I expect this to look is that LinkedClass would get a staticDependencies: Set[ClassName] and a externalDependencies: Set[String] field. (but we should probably introduce a more specific type for externalDependencies).

[sjrd]

Yes, I believe that plan sounds good :)

[gzm0]

I have added a new commit with this. I think it is better.

The only thing I do not like is how we handle module initializers. Maybe we should introduce a LinkedModuleInitializer? It is still not super nice, that the Refiner is re-classifying entry points into modules.

[sjrd]

Couldn't we send each module to Closure in isolation?

[gzm0]

No. The renaming across the modules needs to be consistent. So it might be possible to make this work, but it seems the main use case we want to address here is to feed these things downstream to webpack or other. So IMO we should not try to support this in a first iteration (probably provide better error messages though :)).

[sjrd]

We can potentially extend the Analysis, but now I see that there is a bigger problem: LinkedClass, which is in standard, contains references to Infos as well. That goes beyond intention, and directly into stabler-references-less-stable territory, which is not good at all.

From what I see, those Infos are needed in ModuleAnalyzer.staticDependencies. IIUC, this computes a dependencies: Set[ClassName]. Could we compute these dependencies eagerly in the Infos/Analyzer? Then we could expose that dependencies in Analysis (where it would make sense) and further in LinkedClass (which would therefore not depend on a type less stable than itself).

[sjrd]

Some more comments.

[sjrd]

Consider refactoring as:

Suggested change

	Block(tree, Apply(defProp, List(exportsVarRef, name, descriptor)))
	val export = Apply(defProp, List(exportsVarRef, name, descriptor))
	Block(tree, export)

and then you can factor out val export = and Block(tree, export) outside of the if (mutable).

[sjrd]

It seems to me that the most reasonable outcome would be an empty file. If a module has been requested by the user, and it ends up being empty, why should we get rid of it or consider that an error?

[sjrd]

No, I think what you have now is better. It's better if front-end phases (the optimizer, but also future custom front-end phases) don't have to worry about modules.

[sjrd]

That seems fishy. Should we emit references to other modules as ./foo.js instead of foo.js to avoid the need for NODE_PATH?

[gzm0]

Maybe. I didn't understand that standard well enough. Is that required to work in every engine? (if yes, we should probably indeed do that).

[sjrd]

Nothing is required to work in every engine, if you only read the standard. But if an engine doesn't support explicit relative paths like that, it's unlikely that it supports any kind of reasonable module resolution anyway.

[gzm0]

Hmmm... But then haven't we taken the stance that we do not want to rely on specifics of engines? Maybe we need to have an additional moduleName function (just like jsNameWithSourceMap, or have that one return all of the things we need).

[gzm0]

I think I like this idea much more. We should maybe even do this for the references between source maps and JS file. That way:

• We can provide full backwards compatibility.
• We have a single, simple mechanism to deal with all the oddities of how things reference each other in a given environment.

[gzm0]

@sjrd this is another thing we should discuss: Currently, we theoretically allow assigning to any static field from anywhere. Nothing I know of uses this. It is more difficult to do across ES6 modules because we need a dedicated setter function (and hence the trees look entirely different).

We can of course build it, but maybe only allowing to write to a static field from the context of the class it is defined in would be an acceptable squint-with-the-eyes IR change?

Otherwise, how do we test this :)

[sjrd]

Under optimizations, this is very difficult to ensure. You could have a method that modifies the static field in its class, but if that method is inlined in another class, the assignment will be moved to a different class, and there's not much you can do about it.

Otherwise, how do we test this :)

With an @inline def :p

[gzm0]

Ah, right. Fair enough :)

[gzm0]

Open questions at this point:

• Stick with dependency analysis in Analyzer? (I vote yes)
• Introduce LinkedModuleInitializer?
• What to do with filename patterns / OutputSpec?
• What to do with writing of static fields? (Answer: Allow, under inlining it's necessary).

[gzm0]

Closing in favor of #4198.