Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2681: Module Splitting Support #4198

Merged
merged 7 commits into from Oct 4, 2020
Merged

Conversation

gzm0
Copy link
Contributor

@gzm0 gzm0 commented Sep 16, 2020

No description provided.

@gzm0 gzm0 requested a review from sjrd September 16, 2020 12:17
@gzm0
Copy link
Contributor Author

gzm0 commented Sep 16, 2020

At the moment, the overall diff is identical to #4111

@gzm0
Copy link
Contributor Author

gzm0 commented Sep 16, 2020

Benchmarks for the single module case (i.e. default config).

speed

This shows us that when module splitting is disabled, we do not regress on linking speed after this PR.

@sjrd
Copy link
Member

sjrd commented Sep 19, 2020

I'll probably have to go over this bits by bits ^^

By any chance, would you have some advice on in which order to read things in the big commit (Add Module Splitting support in the linker)?

@gzm0
Copy link
Contributor Author

gzm0 commented Sep 20, 2020

By any chance, would you have some advice on in which order to read things in the big commit (Add Module Splitting support in the linker)?

  1. Review ModuleSet.
  2. Observe (informally, look at code if you need to convince yourself):
    • The top level export trees and module initializers have a (public) moduleID parameter.
    • The linker writes to a directory instead of a single file.
  3. Review LinkerFrontendImpl
  4. Review ModuleSplitter & ModuleAnalyzer.
  5. Review the ModuleAnalyzer implementations.
  6. Review ModuleContext, Emitter.ClassID, Emitter.
  7. Review VarGen.
  8. Review the rest of emitter (boring, mostly more WithGlobal and more implicits).
  9. Review OutputWriter.
  10. At this point, you understand how the splitting works. Review everything else (in whatever order you want).

@sjrd
Copy link
Member

sjrd commented Sep 20, 2020

Thank you! That will be very helpful. :)

@gzm0
Copy link
Contributor Author

gzm0 commented Sep 20, 2020

I forgot about the Analyzer. I suggest you review it after the emitter.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

I reviewed the steps 1 to 9 so far (including 6b for the Analyzer). Overall this looks really excellent! I have very minor comments here and there, but nothing major.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Some more review until linker/jvm/src/test/scala/org/scalajs/linker/PathOutputDirectoryTest.scala, excluded.

@gzm0
Copy link
Contributor Author

gzm0 commented Sep 27, 2020

I have addressed the first round of comments. All questions I have not answered in the review thread should be answered in code comments.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

And that's the end of the review for the big Linker commit.

@gzm0
Copy link
Contributor Author

gzm0 commented Sep 27, 2020

I forgot to mention: I also rebased on master. There was a trivial conflict in Build.scala (adjacent, independent lines modified).

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Reviewed until ScalaJSPluginInternal.scala (excluded) in the sbt plugin commit.

Jenkinsfile Show resolved Hide resolved
project/BinaryIncompatibilities.scala Outdated Show resolved Hide resolved
project/Build.scala Outdated Show resolved Hide resolved
project/Build.scala Outdated Show resolved Hide resolved
project/Build.scala Outdated Show resolved Hide resolved
@gzm0
Copy link
Contributor Author

gzm0 commented Sep 29, 2020

@sjrd There is a subtle bug I have introduced when I moved symbolRequirement handling to the Analyzer. The main reason for this was that it's infeasible to handle optional requirements in the module analyzer (because it needs to make decisions based on method existence). Now in the Analyzer, symbolRequirements also contain module initializers. As such, jl.Object ends up statically depending on module entry points. This is of course total nonsense.

I will attempt to fix the problem by making module initializers first class citizens in the Analyzer. This is unfortunate, but I'm not sure I see another option.

@gzm0
Copy link
Contributor Author

gzm0 commented Sep 30, 2020

I have added a commit fixing the bug and adding a regression test.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

All right, here is the end of the review, including the latest commit that fixes the module initializer issue.

There is one serious issue in ScalaJSPluginInternal (there are a few comments that relate to the same thing), but otherwise it looks very good.

class MinModuleSplittingTest {
import scala.concurrent.ExecutionContext.Implicits.global

/** Smoke test to ensure modules do note get merged too much. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** Smoke test to ensure modules do note get merged too much. */
/** Smoke test to ensure modules do not get merged too much. */

@js.native
private def basename(str: String): String = js.native
object PromisesFS extends js.Object {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
object PromisesFS extends js.Object {
private object PromisesFS extends js.Object {

@@ -1478,6 +1472,19 @@ class ExportsTest {
}
}

@Test def top_level_export_field_is_writiable_accross_modules(): Unit = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Test def top_level_export_field_is_writiable_accross_modules(): Unit = {
@Test def top_level_export_field_is_writable_accross_modules(): Unit = {

@@ -122,6 +131,8 @@ private[sbtplugin] object ScalaJSPluginInternal {
box.ensure(linkerImpl.clearableLinker(config))
},

scalaJSLinker in legacyKey := (scalaJSLinker in key).value,
Copy link
Member

Choose a reason for hiding this comment

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

This is not right. It will silently break compatibility in ways that are hard to debug. Although it's a bit of a specialized use case, if a user had specified

scalaJSLinker in (Compile, fastOptJS) := something

that setting will now be ignored. IMO we should do the reverse. Set scalaJSLinker in legacyKey first, then derive scalaJSLinker in key := (scalaJSLinker in legacyKey).value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Writing to scalaJSLinker is specified to be an unstable API, but fair enough, for consistency with everything else, we might as well do it.

@@ -141,6 +152,8 @@ private[sbtplugin] object ScalaJSPluginInternal {
Tags.Tag(s"uses-scalajs-linker-$projectPart-$configPart-$stagePart")
},

usesScalaJSLinkerTag in legacyKey := (usesScalaJSLinkerTag in key).value,
Copy link
Member

Choose a reason for hiding this comment

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

This one is fine because it is essentially a read-only key from the users' point of view.

@@ -151,6 +164,8 @@ private[sbtplugin] object ScalaJSPluginInternal {
scalaJSLinkerConfigFingerprint in key :=
StandardConfig.fingerprint((scalaJSLinkerConfig in key).value),

moduleName in legacyKey := (moduleName in key).value,
Copy link
Member

Choose a reason for hiding this comment

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

This one is arguably OK because it will still affect the artifactPath in legacyKey. Nevertheless, I think we should also derive the new one from the old one, for consistency.

@@ -335,14 +410,21 @@ private[sbtplugin] object ScalaJSPluginInternal {
((crossTarget in fullOptJS).value /
((moduleName in fullOptJS).value + "-opt.js")),

scalaJSLinkerConfig in fullOptJS ~= { prevConfig =>
scalaJSLinkerConfig in linkJSProd ~= { prevConfig =>
Copy link
Member

Choose a reason for hiding this comment

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

This is the most annoying one, as it is a very real use case for people to tweak scalaJSLinkerConfig in (Compile, fullOptJS). We absolutely must not throw away their configuration, especially since it also affects the result of the legacy keys themselves.

So we should still have here in fullOptJS, and after that add

scalaJSLinkerConfig in linkJSDev := (scalaJSLinkerConfig in fastOptJS).value,
scalaJSLinkerConfig in linkJSProd := (scalaJSLinkerConfig in fullOptJS).value,

@gzm0
Copy link
Contributor Author

gzm0 commented Sep 30, 2020

I have added a commit addressing the comments. (I have changed everything to be in terms of the legacyKey for consistency).

Regarding the names of the new tasks. I like your suggestion from #4111:

Perhaps fastLinkJS and fullLinkJS? I'll keep thinking.

It keeps fast / full but introduces the name link which makes sense anyways. Probably better than introducing dev / prod.

For me, there are two open questions on this PR:

  • Shall we rename linkJSDev / linkJSProd (linkJSFast / linkJSFull might also be options, also in view of autocomplete: fast<tab> would give the old tasks. then again, visibility will probably handle that).
  • Are there enough tests for the legacy integration (notably the one in Linker)?

@sjrd
Copy link
Member

sjrd commented Sep 30, 2020

  • Shall we rename linkJSDev / linkJSProd (linkJSFast / linkJSFull might also be options, also in view of autocomplete: fast<tab> would give the old tasks. then again, visibility will probably handle that).

I would say that I prefer both linkJSFast and fastLinkJS over linkJSDev. I can't really make up my about which is better between linkJSFast and fastLinkJS. The latter sounds (literally) better to me, but that is very subjective.

  • Are there enough tests for the legacy integration (notably the one in Linker)?

For the programmatic API of the Linker, I think that's enough, yes. For the legacy sbt API, I would suggest adding a few more customizations in legacy-link-tasks that would exercise the other keys you fixed in the last commit (scalaJSLinkerConfig in (Compile, fullOptJS) notably).

@gzm0
Copy link
Contributor Author

gzm0 commented Oct 1, 2020

The latter sounds (literally) better to me, but that is very subjective.

I'd tend to agree in fact. It speaks easier.

OTOH, linkJSFast is closer to how sbt tasks like testOnly, testQuick or runMain are named.

From the 6 possible permutations of js, link and fast, I think the following are acceptable:

  • linkJSFast
  • fastLinkJS
  • jsLinkFast
  • jsFastLink

linkFastJS and fastJSLink are bad, because they'd imply the JS is fast.

Personally, I think I prefer linkJSFast because of how it aligns with sbt's tasks and it goes from most to least important property of the task left to right (that argument might also work in favor of jsLinkFast though).

@sjrd
Copy link
Member

sjrd commented Oct 1, 2020

On the other hand, fastLinkJS is more similar with today's fastOptJS ...

Ah, naming in computer science!

@gzm0
Copy link
Contributor Author

gzm0 commented Oct 2, 2020

Let's settle on fastLinkJS then. My plan is to squash the commits and then perform the rename. Then re-push, including the commit-by-commit CI. Is that OK with you?

@sjrd
Copy link
Member

sjrd commented Oct 2, 2020

Sounds good. :)

Without the leading slash, the resulting Path ends up not having a
parent Path / directory. This in turn makes the (upcoming) backwards
compatibility adapter for the Linker fail.

While `Path`s without parents are techincally allowed, its hardly a
case we should care about for the backwards compatibility adapter.
@gzm0
Copy link
Contributor Author

gzm0 commented Oct 2, 2020

Rebased. Testing marathon is starting.

Adding -Xexperimental makes compilation fail if the unapply is not
marked final: "could not optimize @tailrec annotated method unapply:
it is neither private nor final so can be overridden"
This commit does not add support to the compiler nor the sbt-plugin to
test backwards compatibility.

Further, we do not deprecate the old API yet to avoid build failures.
We cannot test this fully yet, since we need support in the sbt-plugin
for that.
At this point, we can fully test exports end-to-end. We do not migrate
all sbt-plugin tests to test backwards compatibility. However, we can
now deprecate the old APIs without build failures.

The custom-linker sbt test is migrated in this commit because it
relies on the internal scalaJSLinkerBox key.
@gzm0
Copy link
Contributor Author

gzm0 commented Oct 4, 2020

@sjrd testing is reaching completion. Do you have further comments on this?

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Let's ship it! 🚀 ✨

@sjrd sjrd merged commit 6622d0b into scala-js:master Oct 4, 2020
@gzm0 gzm0 deleted the module-splitting branch October 4, 2020 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shared modules/progressive apps support
2 participants