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

Experimental support for build pipelining [ci: last-only] #7712

Merged

Conversation

retronym
Copy link
Member

@retronym retronym commented Feb 4, 2019

Supporting infrastracture for build tools that want to implement build pipelining.

This means that in a multi-module builds, a downstream project could be compiled immediately after its upstream projects have finished typechecking (more precisely, have finished the "pickler" phase), rather waiting for the backed of the the compiler to produce .class files.

ClassfileParser is extended to support .sig files, containing just the Scala Pickle (which is usually parsed from the argument of a class file annotation).

A utility is provided to extract .sig files for a given classpath entry. This also strips out code from non-Scala originated classfiles.

PipelineMain is a proof-of-concept driver that demonstrates how this all fits together. It can accept a set of compiler argument files, infer the inter-project dependencies, and schedule pipelined compilation.

@scala-jenkins scala-jenkins added this to the 2.12.9 milestone Feb 4, 2019
@retronym retronym changed the title Experimental support for build pipelining Experimental support for build pipelining [ci: last-only] Feb 4, 2019
@retronym retronym force-pushed the topic/pipeline-trace-macro-omnibus-rebase branch 2 times, most recently from 3c05e29 to cf83014 Compare February 4, 2019 04:49
@jvican jvican self-requested a review February 4, 2019 08:40
@smarter
Copy link
Member

smarter commented Feb 15, 2019

Very cool to see this! For outline typing to work correctly, you'll need to at least disable it for trees that may contain super-accessors, see https://github.com/lampepfl/dotty/pull/4767/files#diff-7c63b7bfffa9340fb48ac9b61fa56beeR1366 for the list of things where it needs to be disabled in Dotty (might not be complete, but was good enough to make parallelization work on Dotty itself as well as a few smaller projects). Also note that the compiler phases that sbt inject are not thread-safe, this was recently fixed but isn't part of any released version of sbt: sbt/zinc#626, (in my PR I worked around this by synchronizing the callbacks, but that's only possible because we inlined the sbt phases in Dotty itself, for scalac you'll need to use a custom scalaCompilerBridgeSource in every sbt build where you want to enable parallelization).

@retronym
Copy link
Member Author

Thanks for the pointer. The exception for super is annoying, but such is life. I think my implementation will work for final val MOL = 42 because non-type ascribed bodies are type checked. Not sure about dotty's exception for:

Lambdas cannot be skipped, because typechecking them may constrain type variables.

Could you elaborate on that with an example?

@smarter
Copy link
Member

smarter commented Feb 15, 2019

Wrote this a while ago so I don't have an example handy, but in Dotty, lambdas are desugared early into a Block node that wraps a DefDef with the body of the lambda and a Closure node, typechecking that DefDef may affect type inference in the enclosing method so it's not a black box, but this check should be refined to lambdas that are enclosed in a method that must be typechecked, and in any case that doesn't matter for Scala 2 since you don't have the same encoding for lambdas.

.sig files, containing the pickle bytes, are output
in place of .class files emitted by Scalac.

Java defined .class files are emitted after stripping
them of code.
If a new hidden setting is enabled. Build tools that want to implement
build pipelining can use this to feed to downstream classpaths.
Under this mode, the RHS of defs and vals are only typechecked if
the result type of the definition is inferred and the definition's
signature is forced.

The movitivation is to create a fast path to extract the pickles for
the API for use on the classpath of downstream compiles, which could
include parallel compilation of chunks of the the current set of
source files.
@retronym retronym force-pushed the topic/pipeline-trace-macro-omnibus-rebase branch from e31e6a8 to b955c4f Compare February 20, 2019 04:00
@retronym
Copy link
Member Author

I think I'll move the -Youtline out of this PR into a separate one. It really warrants some more effort to write the test cases to drive the special cases needed for super-accessors.

@jvican Do you have any comments about the other parts of this PR?

@jvican
Copy link
Member

jvican commented Feb 26, 2019

Oh, I'm sorry for my delay reviewing this, it did fell through the cracks. I'm having a deeper look at it today and will provide more thorough comments.

jvican
jvican previously requested changes Feb 27, 2019
Copy link
Member

@jvican jvican left a comment

Choose a reason for hiding this comment

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

Great to see this making it into the compiler! The experiments I've done with our implementation of pipelined compilation in bloop have been positive overall, giving for some builds such as Spark's > 30% speedup (the implementation is compatible with 2.11, 2.12 and 2.13). However, I'm excited to see an implementation inside the compiler. The compiler is in a much better position to provide a fast and efficient pipelining implementation.

I've left a few comments about the current approach. There are some conflicts between the changes here and some places in the incremental compiler bridge that we should ideally fix so that we can continue to provide the same bridge sources for all minor versions in 2.12 (and not cut a special one for 2.12.9 which would need some changes to the logic that resolves compiler versions).

The implementation seems sounds so I'm just commenting on high-level stuff, providing my insight from the build tool side and specifying what we would need to make this accessible to the whole community of tools.

src/compiler/scala/tools/reflect/ReflectGlobal.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/plugins/Plugins.scala Outdated Show resolved Hide resolved
// sub-project compilation is able to get a cache hit. A more comprehensive solution would be to
// involve build tools in the policy: they could close entries with refcount of zero when that
// entry's JAR is about to be overwritten.
private val deferCloseMs = Integer.getInteger("scalac.filebasedcache.defer.close.ms", 1000)
Copy link
Member

Choose a reason for hiding this comment

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

From my side, I'd like a mechanism to enable this class loader caching but be in charged of the invalidation. I feel it would be better to expose an API here instead of relying on concrete top-level caches that cannot be modified easily from the outside.

Can we add an API that we can override in global that allows us to handle the lifetime of these caches?

With this API, I'd be able to override it in Zinc and use it in Bloop to correctly invalidate these caches and free up the file pointers when I know a user operation will write to an open cached file (e.g. when the project of a compiler plugin is recompiled, when a dependent project that uses straight-to-jar compilation is used, etc etc). This will enable 100% correct usage of class loader caching with zero user intervention, which is what I'm aiming for.

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea was that you could override findMacroClassLoader and findPluginClassloader and DIY caching / invalidation.

Copy link
Member

Choose a reason for hiding this comment

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

Now that this is already present in analyser I'm confident we'll be able to implement our own invalidation. Thanks!


def stripClassFile(classfile: Array[Byte]): OutputFile = {
val input = new ClassNode()
new ClassReader(classfile).accept(input, ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES | ClassReader.SKIP_CODE)
Copy link
Member

Choose a reason for hiding this comment

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

Very nice 👍

Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

Sorry to be nitpicking, I stumbled upon a pattern when traversing the PR diff… I love traverse so much.

I’m super happy to see this coming to scala/scala!

src/compiler/scala/tools/nsc/PipelineMain.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/PipelineMain.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/PipelineMain.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/PipelineMain.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/PipelineMain.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/PipelineMain.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/PipelineMain.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/PipelineMain.scala Outdated Show resolved Hide resolved
  - Scope the build strategies inside object PipelineMain
  - Prefer Future.traverse to Future.sequence
@retronym retronym force-pushed the topic/pipeline-trace-macro-omnibus-rebase branch from a17594d to 0b1974c Compare February 28, 2019 02:59
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Mar 1, 2019
We need to typecheck the RHS of type-ascribed definitions if they
contain Super trees that require super-accessors to be added to the
enclosing template. Rather than take that on right now, I'm removing
this feature to focus on the other form of build pipelining.
@retronym retronym dismissed jvican’s stale review March 4, 2019 03:53

All review points addressed.

@retronym retronym merged commit 2961eda into scala:2.12.x Mar 4, 2019
SethTisue added a commit to SethTisue/scala that referenced this pull request Mar 7, 2019
small followup to scala#7712. the community build found that a couple of
projects (mima, twotails) were using the old constructor. since
it's easy to do, let's keep both source compat (with the default
arguent) and bincompat (with the extra constructor, which we can
toss for 2.13)
SethTisue added a commit to SethTisue/scala that referenced this pull request Mar 7, 2019
small followup to scala#7712. the community build found that a couple of
projects (mima, classpath-shrinker) were using the old
constructor. since it's easy to do, let's keep both source compat
(with the default arguent) and bincompat (with the extra constructor,
which we can toss for 2.13)
SethTisue added a commit to SethTisue/scala that referenced this pull request Mar 7, 2019
small followup to scala#7712. the community build found that a couple of
projects (mima, classpath-shrinker) were using the old
constructor. since it's easy to do, let's keep both source compat
(with the default arguent) and bincompat (with the extra constructor,
which we can toss for 2.13)
@SethTisue
Copy link
Member

SethTisue commented Mar 7, 2019

@retronym this caused some easily-fixable-looking community build breakage, prospective fix submitted at #7822

but it also caused the following, which I don't know what to make of. this happens while compiling the plugin's tests, and it's reproducible outside dbuild just by cloning the twotails repo and doing ; ++2.12.9-bin-88ed07f! ; Test/compile

looking at the twotails build, the only thing unusual I see is:

  scalacOptions in Test ++= Seq(
    "-Xplugin:" + (packageBin in Compile).value,

@retronym
Copy link
Member Author

retronym commented Mar 7, 2019

@SethTisue #7825

SethTisue added a commit to SethTisue/scala that referenced this pull request Mar 7, 2019
restore source compat and bincompat

small followup to scala#7712. the community build found that a couple of
projects (mima, classpath-shrinker) were using the old
constructors. since it's easy to do, let's keep both source compat
(with the default arguments) and bincompat (with the extra
constructors, which we can toss for 2.13)
SethTisue added a commit to SethTisue/scala that referenced this pull request Mar 7, 2019
restore source compat and bincompat

small followup to scala#7712. the community build found that a couple of
projects (mima, classpath-shrinker) were using the old
constructors. since it's easy to do, let's keep both source compat
(with the default arguments) and bincompat (with the extra
constructors, which we can toss for 2.13)
@eed3si9n eed3si9n mentioned this pull request Aug 1, 2019
2 tasks
eed3si9n added a commit to eed3si9n/zinc that referenced this pull request Mar 3, 2020
@eed3si9n eed3si9n mentioned this pull request Mar 3, 2020
3 tasks
eed3si9n added a commit to eed3si9n/zinc that referenced this pull request Mar 3, 2020
eed3si9n added a commit to eed3si9n/zinc that referenced this pull request Mar 11, 2020
eed3si9n added a commit to eed3si9n/zinc that referenced this pull request Mar 23, 2020
eed3si9n added a commit to eed3si9n/zinc that referenced this pull request Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
6 participants