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

Compile to jar, performance improvements #453

Closed
wants to merge 15 commits into from

Conversation

lukaszwawrzyk
Copy link
Contributor

@lukaszwawrzyk lukaszwawrzyk commented Jan 24, 2019

This PR integrates new zinc and 2 features it introduced: compiling to jar file sbt/zinc#597 and specifying ignored scalac options sbt/zinc#548. It also has multiple smaller improvements mostly related to general build performance.

Note that I opened this PR now to allow review, suggesting changes, etc. It is not ready to merge because zinc is not yet released (the available version 1.3.0-M1 has a bug, that is already fixed and merged, but we will need wait for the release). I may add some more independent peformance improvements, but potentially on a different PR. I don't plan to change the code that is here, unless required by the review.

Compile to jar

Most of the details are in the zinc PR. Basically zinc was adjusted to be able to call scalac with output specified as .jar file, which means that classes are written directly to a jar file instead of multiple class files. This can give notable performance improvements, mostly on windows, that has poor performance wrt e.g. creating multiple small files.
The current integration is not full and perfect, but it works. Before invoking zinc, the output directory is replaced with <output directory>.jar. Resources are still copied to the output directory. To allow running apps, the .jar file has to be added to the classpath of the module, which is handled by CompileToJarComponent.
There are 2 known problems:

  • If the output jar does not exist at all and user runs the app (which triggers build) the application will fail with class not found. This is because intellij's file system doesn't seem to realize fast enough that the jar appeared before running app, and it is not included in the classpath. Second and further runs work as expected.
  • Switching between compile to jar and to classes back and forth is not seamless. If project was compiled with zinc to classes and then users tries to compile it to jar, zinc will pick up analysis files from regular compilation and fail with error on them. This means that the switch should be followed by cleaning output directories (rebuild does not seem to remove zinc analysis files). But in general this is one time operation, users shouldn't switch too often between those modes.

Ignored scalac options

Normally if a scalac option is changed (added, removed) this means that zinc would trigger full compilation (not incremental). Some of the options though do not impact the bytecode. This integration allows to specify which option changes should be ignored. For example -Xprint:typer.
Both this and compile to jar features were put in a new settings panel under Build, Execution, Deployment > Compiler > Scala Compiler > Zinc (right next to Scala Compile Server), as those are currently related to zinc only and tbh there is no room for them anywhere else.


THIS PROGRAM IS SUBJECT TO THE TERMS OF THE BSD 3-CLAUSE LICENSE.

THE FOLLOWING DISCLAIMER APPLIES TO ALL SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE:
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS “AS IS” AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA,
OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR
ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.
ONLY THE SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE, IF ANY, THAT ARE ATTACHED TO (OR OTHERWISE ACCOMPANY) THIS SUBMISSION (AND ORDINARY
COURSE CONTRIBUTIONS OF FUTURES PATCHES THERETO) ARE TO BE CONSIDERED A CONTRIBUTION. NO OTHER SOFTWARE CODE OR MATERIALS ARE A CONTRIBUTION.

@jastice jastice requested a review from niktrop January 24, 2019 12:25
Copy link
Contributor

@niktrop niktrop left a comment

Choose a reason for hiding this comment

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

Thank you for contribution!
Please sign our Contributor Licence Agreement:
https://www.jetbrains.com/agreements/cla/

override def buildStarted(context: CompileContext): Unit = {
if (isScalaProject(context) && !isDisabled(context)) {
override def chunkBuildStarted(context: CompileContext, chunk: ModuleChunk): Unit = {
if (isScalaProject(context) && !isDisabled(context) && hasScalaModules(context, chunk)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't zinc need to compile java files to collect incremental compilation data? I thought it's necessary to recompile dependencies on a change of some base java module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will analyze that tomorrow, thanks for pointing out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I checked that. So it actually fixes a bug. If in scala project you create a purely java module without scala sdk, it will error with No scalac found to compile scala sources: ... (from org/jetbrains/jps/incremental/scala/local/SbtCompiler.scala:23). So if zinc is to be used, it needs scala sdk as well. After this fix, the module would be compiled with regular java builder (no zinc involved).

Copy link
Collaborator

@pavelfatin pavelfatin Feb 20, 2019

Choose a reason for hiding this comment

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

If in scala project you create a purely java module without scala sdk, it will error with No scalac found to compile scala sources: ... (from org/jetbrains/jps/incremental/scala/local/SbtCompiler.scala:23). So if zinc is to be used, it needs scala sdk as well.

Hm... that doesn't look correct - it seems that it should be possible to compile Java-only modules, without Scala SDK attached. How does SBT compile Java-only modules?

After this fix, the module would be compiled with regular java builder (no zinc involved).

But... that will prevent the to-JAR compilation. What if one expects to find a JAR as a result? Besides, that "Incrementailty Type" compilation setting in IDEA is project-wide, not per-modules, so such a behavior would be unexpected. I also wonder whether incremental compilation will work in such a case, in both directions - we need to verify that.

(Speaking of incremental compilation - it supposed to be integrated with other languages / compilers / GUI forms in IDEA. What happens if we store the output classes in a JAR? It seems that we need to list specific use cases and test each of them to make sure that everything works.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... that doesn't look correct - it seems that it should be possible to compile Java-only modules, without Scala SDK attached. How does SBT compile Java-only modules?

Probably sbt is implemented differently to always pass those jars or something. I don't want to test again, I am pretty sure it was like that because I was testing it when I was writing the original comment. Try to create some scala project and add java module that does not depend on it and does not have scala sdk and try to build it with zinc. It should fail with the error I mentioned. After the fix it will be built without zinc.

But... that will prevent the to-JAR compilation. I also wonder whether incremental compilation will work in such a case, in both directions - we need to verify that.

Yes, it prevents compilation to jar and incremental compilation, but aside from that it produces output, you can run apps from there and so on. If one wants zinc features, one must add scala sdk.
Is there any other solution you were thinking about?

(Speaking of incremental compilation - it supposed to be integrated with other languages / compilers / GUI forms in IDEA. What happens if we store the output classes in a JAR? It seems that we need to list specific use cases and test each of them to make sure that everything works.)

There are two areas of that. I am not sure which are you asking about.

  • Integrating compilation to jar with language / compiler
    Highly depends on how incremental compiler work. The example of such integration is in my zinc PR mentioned in first post. It involves appending new classes to jar, pruning invalidated classes, etc. We operate on a single jar instead of the file system. There are multiple tricks used there to keep it efficient.
  • Integrating compile to jar with intellij
    For the use-cases we have it is mostly building and running apps. Both handled in this PR. For running we had to add jars to classpath. Resources are still copied normally (not included in jar) so we have 2 classpath entries instead of 1.
    The other thing is running tests. AFAIR the logic in intellij for junit that we were used is just skipping jars explicitly when looking for tests. We have it solved by setting java property junit4.search.4.tests.in.classpath=false so that it falls back to different method of looking for tests. We were not looking at things like packaging/generating artifacts, gui forms.
    I also recall one more thing. If user is running an app but at the same time wants to edit and compile the code that the app uses it will fail, as jar will be locked. As a workaround I implemented a logic that when app is run, creates a copy of the compiled jar with _runtime suffix. Only the runtime jars are used for running. I check the timestamps to avoid overhead. It is quite performant even on windows. I hooked it into RunConfigurationExtension#updateJavaParameters for our custom run configurations that we are using for all apps. I am not sure how could it be implemented in general case.

private val moduleCacheFileSuffix = "_options-cache.txt"

/**
* @return true if compiler options change was detected
*/
def updateCompilerOptionsCache(context: CompileContext, chunk: ModuleChunk, moduleNames: Seq[String]): Boolean = {
val scalacOptsCacheFile = getCacheFileFor(context, moduleNames)
val scalacOptsCacheFile = getCacheFileFor(context, chunk.representativeTarget())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure representativeTarget is guaranteed to be the same between compilations. It would be more safe to sort them by name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks as potentially unstable. Nice catch. I fixed that.

def deserializeEvent(data: Array[Byte]): Event = {
Event.fromBytes(decompress(fromBase64(data)))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Does compressing have measurable effect on performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were doing optimizations for build time. There was no one big thing. Rather multiple small things, and this is one of them. I don't have numbers, but it was something like couple % or more of execution time of JPS, reduced down to less than 1 wrt socket communication. Most of the small messages are not compressed by much, but big ones have their sizes reduced like 7-13 times. There are 2 big things sent now: the arguments for the nailgun that contain e.g. full classpath (that for us was e.g. 400 entries) sources, etc. I merged that into one message and compressed. The other event is introduced in compile to jar part - AllGenerated with generated class files. This also reduced overhead on sockets, instead of sending many small events, we send one and it is compressed as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing is that compression / decompression could also reduce the performance, especially, considering that the I/O is local. We do need benchmarks to verify the improvement.

Most of the small messages are not compressed by much, but big ones have their sizes reduced like 7-13 times.

But the size reduction is irrelevant, it's not that we're trying to save local traffic.

This also reduced overhead on sockets, instead of sending many small events, we send one and it is compressed as well.

Again, we do need the numbers here, and separately from the compression. It's reasonable to modify things only if that actually improves something.

In any case, it's sensible to extract that modification in a separate PR.

(BTW, we're considering using protobuf for the protocol)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will see if I have time for benchmarks. First thing to mention is that while we were working on that we were only working on windows.
I was looking at jprofiler output, trying to find hotspots and then find a way to reduce time spent in them. Each change was giving an improvement. I wasn't storing those results.
IO is local, but local sockets still aren't free, their impact was noticable. We reduced both number of messages and amount of data.

Protobuf would be nice in general, though I am not sure how much would it help with performance. As I understand nailgun and how it is implemented, we s till need a string, which enforces base64. Also protobuf does not compress messages AFAIK.

@@ -80,17 +87,19 @@ abstract class BaseCompilationData extends CompilationDataFactory {

val outputGroups = createOutputGroups(chunk)

val canonicalSources = sources.map(_.getCanonicalFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

getCanonicalFile was added to prevent case sensitive check in scalac to fail, see https://youtrack.jetbrains.com/issue/SCL-8384.
Why did you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will revert this commit then. We just had no such issue and getCanonicalFile is rather costly in terms of performance (for sure on windows).

@@ -48,6 +48,9 @@ case class TraceEvent(message: String, lines: Array[String]) extends Event
@SerialVersionUID(-3155152113364817122L)
case class GeneratedEvent(source: File, module: File, name: String) extends Event

@SerialVersionUID(-3155152456364817122L)
case class AllGeneratedEvent(generated: Seq[(File, Seq[(File, String)])]) extends Event
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't serializing arrays be more efficient than scala Seqs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggestion, it's definitely worth trying.

Copy link
Contributor

Choose a reason for hiding this comment

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

You may also try to use flat array of Tuple3 instead of nested data if you compress it anyway. It seems it will allow to avoid some collection transformations in SbtCompiler.doCompile.

@niktrop
Copy link
Contributor

niktrop commented Jan 24, 2019

I've also got an exception on first Rebuild Project after enabling "Compile to jar":

Error:scala: ## Exception when compiling 1 sources to C:\Users\Nikolay.Tropin\IdeaProjects\untitled4\target\scala-2.12\classes.jar
scala.MatchError: [Ljava.lang.String;@302c5797 (of class [Ljava.lang.String;)
sbt.internal.inc.JarUtils$ClassInJar$.splitJarReference$extension(JarUtils.scala:39)
sbt.internal.inc.JarUtils$ClassInJar$.toClassFilePath$extension(JarUtils.scala:37)
sbt.internal.inc.ClassFileManager$DeleteClassFileManagerForJar.$anonfun$delete$5(ClassFileManager.scala:176)
scala.collection.TraversableLike.$anonfun$map$1(TraversableLike.scala:233)
scala.collection.IndexedSeqOptimized.foreach(IndexedSeqOptimized.scala:32)
scala.collection.IndexedSeqOptimized.foreach$(IndexedSeqOptimized.scala:29)
scala.collection.mutable.ArrayOps$ofRef.foreach(ArrayOps.scala:194)
scala.collection.TraversableLike.map(TraversableLike.scala:233)
scala.collection.TraversableLike.map$(TraversableLike.scala:226)
scala.collection.mutable.ArrayOps$ofRef.map(ArrayOps.scala:194)
sbt.internal.inc.ClassFileManager$DeleteClassFileManagerForJar.delete(ClassFileManager.scala:176)
sbt.internal.inc.IncrementalCommon$.pruneClassFilesOfInvalidations(IncrementalCommon.scala:742)
sbt.internal.inc.Incremental$.prune(Incremental.scala:136)
sbt.internal.inc.IncrementalCompilerImpl.compileInternal(IncrementalCompilerImpl.scala:325)
sbt.internal.inc.IncrementalCompilerImpl.$anonfun$compileIncrementally$1(IncrementalCompilerImpl.scala:296)
sbt.internal.inc.IncrementalCompilerImpl.handleCompilationError(IncrementalCompilerImpl.scala:164)
sbt.internal.inc.IncrementalCompilerImpl.compileIncrementally(IncrementalCompilerImpl.scala:244)
sbt.internal.inc.IncrementalCompilerImpl.compile(IncrementalCompilerImpl.scala:70)
org.jetbrains.jps.incremental.scala.local.SbtCompiler.$anonfun$doCompile$1(SbtCompiler.scala:90)
scala.util.Try$.apply(Try.scala:209)
org.jetbrains.jps.incremental.scala.local.SbtCompiler.doCompile(SbtCompiler.scala:88)
org.jetbrains.jps.incremental.scala.local.SbtCompiler.compile(SbtCompiler.scala:21)
org.jetbrains.jps.incremental.scala.local.LocalServer.compile(LocalServer.scala:35)
org.jetbrains.jps.incremental.scala.remote.Main$.make(Main.scala:72)
org.jetbrains.jps.incremental.scala.remote.Main$.nailMain(Main.scala:25)
org.jetbrains.jps.incremental.scala.remote.Main.nailMain(Main.scala)
sun.reflect.GeneratedMethodAccessor2.invoke(Unknown Source)
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
java.lang.reflect.Method.invoke(Method.java:498)
com.martiansoftware.nailgun.NGSession.run(NGSession.java:319) 

@lukaszwawrzyk
Copy link
Contributor Author

@niktrop Thanks for the review.
The error you mention is the second known problem that I listed in the PR description. Old zinc metadata files need to be deleted (best to simply remove output directory) before running compile to jar build.

@lukaszwawrzyk lukaszwawrzyk force-pushed the compile-to-jar branch 2 times, most recently from 7f73531 to 9a9e385 Compare January 25, 2019 11:18
@lukaszwawrzyk
Copy link
Contributor Author

lukaszwawrzyk commented Jan 25, 2019

@niktrop Actually fixing the issue with rebuild after switch was not that difficult. I will open a small PR to zinc.

Edit: sbt/zinc#633

@lukaszwawrzyk
Copy link
Contributor Author

@niktrop @pavelfatin I updated PR with latest zinc 1.3.0-M2 that contains the 2 relevant fixes for this PR.

@romanowski
Copy link
Contributor

@niktrop @pavelfatin Is there anything that blokcs this PR from being merged?

@niktrop
Copy link
Contributor

niktrop commented Feb 19, 2019

@romanowski We are waiting for @pavelfatin review, he is not available right now. I think it will be merged to 2019.1 soon.

@pavelfatin
Copy link
Collaborator

First of all, we understand that it takes time and effort to do something of that scale. Thank you for your contribution, we appreciate it!

The suggested changes seem reasonable, and we're eager go include them, but we do need to make sure that every detail is correct if we're to merge the code. This kind of modification potentially affects many seemingly unrelated things, and it's just too easy to break something unknowingly.

As lot of stuff is mixed into a single PR, it's rather hard to makes sense of the changes.

Could we please, if possible, split this PR into multiple, more focused PRs, for example:

  1. Zinc upgrade.
  2. Optimization of Scalac option check.
  3. Compilation to JAR.
    n. Other improvements, each in a dedicated PR.

In principle, we can do to that by ourselves, but since you already understand those details, it would be great if you do that.

As for the compilation to a JAR file - that looks like a neat feature, yet I wonder what's the actual speedup in practice - is there any numbers? If there's a significant gain, we may consider implementing that feature universally, in the Scala plugin, or even in the IDEA itself. Then the integration with Zinc counterpart should be more smooth, both code- and UX-wise.

It would be great to see non-synthetic numbers, not just for Zinc, but for Zinc in IDEA (and without the unrelated modifications, such as the compression, etc.), for example:

  1. Full recompilation of the Scala plugin project.
  2. Incremental compilation of a 10-15 files in that project.

The Enable straight-to-jar compilation Zinc pull request mentions "a feature regression: multi-module incremental compilation simply does not work with compile-to-jar enabled" - is that so (and if so, if that solved)?

@pavelfatin
Copy link
Collaborator

pavelfatin commented Feb 20, 2019

Before invoking zinc, the output directory is replaced with .jar

The output directories are marked as "excluded" in IDEA project, so that IDEA won't index them. We need to prevent IDEA from indexing the JARs with the generated class files.

Particularly, it's not uncommon to specify ./out and ./testOut output directories. In such a case, out.jar and testOut.jar would be placed in the project base directory, which is not excluded.

How about placing the JARs in the output directory? Something akin to ./out/classes.jar. Then the files will be excluded. Besides, the compiler output path in the module settings will be correct - anyone who, say, checks for the presence of, or deletes those directories will still be able to do that (BTW, there's a "Clean output directory on rebuild" general compilation setting in IDEA).

Besides, IDEA separates between production and test directories - what about that? What if one set the same directories both for production and tests - will this still work with a JAR?

@pavelfatin
Copy link
Collaborator

pavelfatin commented Feb 20, 2019

This integration allows to specify which option changes should be ignored. For example -Xprint:typer.
Both this and compile to jar features were put in a new settings panel under Build, Execution, Deployment > Compiler > Scala Compiler > Zinc (right next to Scala Compile Server), as those are currently related to zinc only and tbh there is no room for them anywhere else.

It's hard to imagine why someone would need to edit that list, and the dedicated settings page is probably too much for such a thing - maybe just hard-code the options?

As for the "compile to JAR" - we can probably show a corresponding checkbox after the "Incrementality Type" combobox, when "Zinc" is selected.

@jastice
Copy link
Member

jastice commented Feb 20, 2019

If the output jar does not exist at all and user runs the app (which triggers build) the application will fail with class not found. This is because intellij's file system doesn't seem to realize fast enough that the jar appeared before running app, and it is not included in the classpath. Second and further runs work as expected.

This seems to be a major problem. We should either notify IDEA about the generated file, or to refresh FS state. We'll look into that.

This keeps popping up in many constellations. Here's an issue: https://youtrack.jetbrains.com/issue/SCL-13401

@lukaszwawrzyk
Copy link
Contributor Author

The suggested changes seem reasonable, and we're eager go include them, but we do need to make sure that every detail is correct if we're to merge the code. This kind of modification potentially affects many seemingly unrelated things, and it's just too easy to break something unknowingly.

When creating this PR compile-to-jar was rather meant as more experimental feature that needs some polishing, but generally works. It is behind a flag/setting, shouldn't break anything unless it is enabled. TBH it is quite unlikely that we will invest time into integrating it with all potentially exotic features of intellij. All other changes should be fine i.e. complete, non-experimental.

As lot of stuff is mixed into a single PR, it's rather hard to makes sense of the changes.

I took quite some time already to reorganize all these changes into separate commits. Looking through commits instead of everything shouldn't be confusing. Most of the commits are independent, but some depend on other. It is hard to split that into proper distinct PRs.
I would maybe think on splitting this PR into things that can be merged and things that need to be somehow reworked, but it would be best to avoid that if possible.

As for the compilation to a JAR file - that looks like a neat feature, yet I wonder what's the actual speedup in practice - is there any numbers? If there's a significant gain, we may consider implementing that feature universally, in the Scala plugin, or even in the IDEA itself. Then the integration with Zinc counterpart should be more smooth, both code- and UX-wise.

I probably did not store these numbers. We were mostly seeing some improvement in scalac phase that writes class files on windows. In general compilation time it was just something like couple %. It is also way faster to e.g. clean the outputs on windows, as deleting many small files is slow. Similarly collecting timestamps is faster on windows when we read them from jar.
Most significant improvement is when we use CachedCompilationProvider. Downloading cache without compile to jar would require to also extract all class files. Compiling to jar speed up this process even like 5-8 times.

The Enable straight-to-jar compilation Zinc pull request mentions "a feature regression: multi-module incremental compilation simply does not work with compile-to-jar enabled" - is that so (and if so, if that solved)?

This is an issue in mill I think. This is rather some issue with integration that needs to be debug, no idea what it may be caused by. We have a working integration with intellij (this PR) and with gradle.

@lukaszwawrzyk
Copy link
Contributor Author

Ideally, we need to do that automatically, in the same way as we handle switching between Zinc / IDEA incremental compilers.
After the fix in zinc it just works even with old analysis.

Besides, IDEA separates between production and test directories - what about that? What if one set the same directories both for production and tests - will this still work with a JAR?

It is not possible to do. When building it shows an error: "Production and test output paths are shared in " The title of the popup suggest that it is scala specific thing.

How about placing the JARs in the output directory? Something akin to ./out/classes.jar. Then the files will be excluded. Besides, the compiler output path in the module settings will be correct - anyone who, say, checks for the presence of, or deletes those directories will still be able to do that (BTW, there's a "Clean output directory on rebuild" general compilation setting in IDEA).

That sounds reasonable.

It's hard to imagine why someone would need to edit that list, and the dedicated settings page is probably too much for such a thing - maybe just hard-code the options?

I am not sure about that. I feel like it is safer to leave it configurable. Scalac options may change, their defaults may change. Also there can be custom flags. For example someone might be developing and testing a new flag of scalac. Also there are flags that configure plugins and there is no way to know how they should be handled.
If list is a problem I would consider doing similar thing as for "Additional compiler options" (an expandable text field).

As for the "compile to JAR" - we can probably show a corresponding checkbox after the "Incrementality Type" combobox, when "Zinc" is selected.

That was my initial implementation, just a checkbox next to incrementality type. But then for the reasons I mentioned above I needed a list as well for ignored options. So I created a new panel. I don't know how to make panel conditionally visible in the tree though.

@pavelfatin
Copy link
Collaborator

I truly believe that fine-grained PRs would be easier to understand, to discuss, and to merge (so that we can merge each of the parts as soon as it's ready).

@pavelfatin
Copy link
Collaborator

A "couple %" is withing a margin of error, not exactly a "notable performance improvement". Typical users shouldn't enable this mode to increase compilation speed. The mode also cannot be used to pre-pack JARs (instead of using the IDEA's "Artifacts"), because some files (e.g. resources, java-only classes) are not included. It seems that, in practice, the mode is only useful in conjunction with the CachedCompilationProvider, so it's better to make that clear in the description.

@pavelfatin
Copy link
Collaborator

BTW, what is the speedup in absolute terms -- the "5-8 times" sounds like a lot, but if it's not about compilation as a whole, but only about the downloading, that might be not much.

Besides, did you downloaded & extracted the data in the optimal way, that is:

  1. Download a single archive per output directory, rather than each .class file separately.
  2. Extract the archive on-the-fly, without saving it to disk. That's how you can perform those two steps in parallel, so that the disk is used only for writing, not to read + write at the same time.
  3. Download & extract each of the archives separately. Assuming that the process is disk-bound, extracting multiple archives simultaneously would takes toll on the FS.

Given the insane speed of modern SSD, and their aptness in random-access I/O, it seems that the above implementation should be actually network-, rather than disk-bound.

@pavelfatin
Copy link
Collaborator

pavelfatin commented Mar 7, 2019

Another consideration is FS caching -- although batch writing and deleting of files is faster with archives, OS caching of FS metadata works for files, but not for archives. Unless you maintain a custom RAM cache of the JAR contents (which is not possible on a one-time compilation, without a compile server), random-access of the archived files might be significantly slower.

For example, what if you need to incrementally compile a single .scala file, that depends on 10 .class files in different 10 modules. Will you need to open and read the metadata of 10 the JAR archives for that?

@pavelfatin
Copy link
Collaborator

pavelfatin commented Mar 7, 2019

Yes, it prevents compilation to jar and incremental compilation, but aside from that it produces output, you can run apps from there and so on.

But... the loss of incremental compilation is not a small thing.

If one wants zinc features, one must add scala sdk.

There's no technical reasons for that. Scala libraries are needed to compile Scala code, that's true. Scala libraries are also needed at runtime, for the Zinc itself -- that's also true. But Scala libraries are not needed (at the compilation classpath) to compile Java code.

Is there any other solution you were thinking about?

The original Zinc implementation says:

If only Java sources are being compiled then the -java-only option can be added to avoid the Scala library jar being automatically added to the classpath.

SBT also seems to handle Java-only modules somehow (we need to look into that). It would be much better to implement this properly and consistently, via Zinc.

(BTW, how otherwise the compilation of Java-only modules are supposed to work outside of IDEA?)

@pavelfatin
Copy link
Collaborator

pavelfatin commented Mar 7, 2019

I am not sure about that. I feel like it is safer to leave it configurable. Scalac options may change, their defaults may change. Also there can be custom flags. For example someone might be developing and testing a new flag of scalac. Also there are flags that configure plugins and there is no way to know how they should be handled.

If list is a problem I would consider doing similar thing as for "Additional compiler options" (an expandable text field).

A configurable list for such a small thing is an over-compication of the UI, but that's sort of OK. A different thing is a completely new settings page, specifically for "Zinc". This is potentially confusing, especially given that Zinc mode is optional, and that very few people actually understand how incremental compilation works (and what "Zinc" is, to begin with, apart from the "marketing").

That was my initial implementation, just a checkbox next to incrementality type. But then for the reasons I mentioned above I needed a list as well for ignored options. So I created a new panel. I don't know how to make panel conditionally visible in the tree though.

Showing / hiding a setting page depending on some control in some other page is not ideal, from the UX standpoint.

Here's a possible solution: there's no technical reason to implement this feature only for Zinc, so we can make it a universal feature, place the list (or, even better, a field -- IDEA supports fields with a pop-up iterm-per-line editor) to the common settings, and then we can use the "a checkbox next to incrementality type" just fine.

@pavelfatin
Copy link
Collaborator

pavelfatin commented Mar 7, 2019

With regards to the requests merging and the compression in the protocol: we don't have any numbers, and it seems that request merging won't increase the I/O in any meaningful way, and so is unnecessary, whereas the compression might actually slow down the I/O (or, at least, consume the CPU), and so is redundant.

@pavelfatin
Copy link
Collaborator

I also recall one more thing. If user is running an app but at the same time wants to edit and compile the code that the app uses it will fail, as jar will be locked. As a workaround I implemented a logic that when app is run, creates a copy of the compiled jar with _runtime suffix. Only the runtime jars are used for running. I check the timestamps to avoid overhead. It is quite performant even on windows. I hooked it into RunConfigurationExtension#updateJavaParameters for our custom run configurations that we are using for all apps. I am not sure how could it be implemented in general case.

This might still result in significant slowdown for modules with lots of files. Ideally, the updating should be done incrementally, so that if there's only one class file changed, you would only need to add a single file to the second archive. We do (did?) something like that for the Scala plugin build, so you may look into that.

@pavelfatin
Copy link
Collaborator

There seem to be various drawbacks to the "compile-to-JAR" scheme: the loss of incrementality for Java-only modules, potential slowdown of incremental compilation, the need for copying of the JARs, potential incompatibilities, potential UX confusion. While I'm not against the idea per se, I wonder whether the (unknown) speedup of the initial JAR downloading (only when using CachedCompilationProvider) really outweighs all the drawbacks, and whether there is a different way to speedup the initial downloading & extractions (e.g., as described above).

For all that, the effectiveness of the scheme as such is out of scope of this PR. If you find the scheme useful, so be it. But we do need to make sure that the modification is reasonable and cohesive (yet this doesn't mean that everything should be 100% perfect and compatible with every single feature of IDEA -- some reasonable compromises are totally OK).

When creating this PR compile-to-jar was rather meant as more experimental feature that needs some polishing, but generally works. It is behind a flag/setting, shouldn't break anything unless it is enabled.

A feature that is not enable by default, but can be enabled via the standard UI is not considered "experimental" (at least, unless explicitly stated so). Besides, some modifications affect the non-optional mechanisms. So, we still need to be careful and don't break anything unnecessarily.

@lukaszwawrzyk
Copy link
Contributor Author

I truly believe that fine-grained PRs would be easier to understand, to discuss, and to merge (so that we can merge each of the parts as soon as it's ready).

How about you list the commits that could be merged without discussing much and I will first create a PR with them so we can get that merged?

A "couple %" is withing a margin of error, not exactly a "notable performance improvement". Typical users shouldn't enable this mode to increase compilation speed. The mode also cannot be used to pre-pack JARs (instead of using the IDEA's "Artifacts"), because some files (e.g. resources, java-only classes) are not included. It seems that, in practice, the mode is only useful in conjunction with the CachedCompilationProvider, so it's better to make that clear in the description.

You are mostly right about that. I rather meant that improvement can be observed. On large module if you print time for scalac phases, the jvm phase is shorter. This can be in order of seconds.

BTW, what is the speedup in absolute terms -- the "5-8 times" sounds like a lot, but if it's not about compilation as a whole, but only about the downloading, that might be not much.

Yes it is about a case when there are no changes in the code and we only need to download cache and run zinc to see if there are no changes. The requirement to unzip all classes vs just keep jared classes is the significant part here that makes all the difference.

Both compile-to-jar and .class file approach are fairly optimal. We download 1 jar per output in ctj case, and that's about it. For .class files, jar is extracted directly from remote location to the output directory.
SSDs are fast, but windows is not great, it is not an issue on linux. Just try to extract a large jar on windows vs linux. This makes all the difference.

@lukaszwawrzyk
Copy link
Contributor Author

Another consideration is FS caching -- although batch writing and deleting of files is faster with archives, OS caching of FS metadata works for files, but not for archives. Unless you maintain a custom RAM cache of the JAR contents (which is not possible on a one-time compilation, without a compile server), random-access of the archived files might be significantly slower.

I am not sure if I understand. What is the FS caching used for and what problems can it cause?

For example, what if you need to incrementally compile a single .scala file, that depends on 10 .class files in different 10 modules. Will you need to open and read the metadata of 10 the JAR archives for that?

As far as I understand this is handled by zinc, or rather scalac itself. The class files of dependencies will be passed as jars instead of directories. Any caching is handled in scalac, and scalac is quite good with using jars. Note that most of the things on classpath for scalac are jars. We didn't identify any slow down on that.

But... the loss of incremental compilation is not a small thing.

It is not a loss. We can't lose something we don't have. As I explained earlier (and gave a way to reproduce) in that case we have nothing. We don't have incremental compilation, or any compilation at all, we have an error. So having regular compilation vs error seems to be an improvement.

Other than that you are right. The fix in this PR is more of a hack, and it should be fixable properly, thoguh needs more investigating. There is no -java-only flag in zinc anymore. Not sure if I will have time to investigate, but we will see.

@lukaszwawrzyk
Copy link
Contributor Author

Here's a possible solution: there's no technical reason to implement this feature only for Zinc, so we can make it a universal feature, place the list (or, even better, a field -- IDEA supports fields with a pop-up iterm-per-line editor) to the common settings, and then we can use the "a checkbox next to incrementality type" just fine.

Where exactly should that field go (which panel etc.)?

@lukaszwawrzyk
Copy link
Contributor Author

With regards to the requests merging and the compression in the protocol: we don't have any numbers, and it seems that request merging won't increase the I/O in any meaningful way, and so is unnecessary, whereas the compression might actually slow down the I/O (or, at least, consume the CPU), and so is redundant.

I see that commit also as a good refactoring of what was happening there earlier. How about merging that except for the compression (remove that step in transformation pipeline in the Protocol class). It is still an improvement. From our perspective it means less conflits on updating plugin at least.

I am not sure what do you mean by "it seems that request merging won't increase the I/O in any meaningful way, and so is unnecessary," By request merging you mean merging arguments into one? It reduces number of send calls to socket api.

@lukaszwawrzyk
Copy link
Contributor Author

This might still result in significant slowdown for modules with lots of files. Ideally, the updating should be done incrementally, so that if there's only one class file changed, you would only need to add a single file to the second archive. We do (did?) something like that for the Scala plugin build, so you may look into that.

But the thing is that there is just one output file per module, the jar. It was efficient enough, even on windows, that we just copied the whole jar to not overengeneer this.

As you mention scala plugin build I have issues with that. It happens quite often that after invoking package task, it compiles the changed classes, though they are not included in the jars, e.g. for changes in compiler-jps, deleting that jar from target and running package again helps. Is that know issue?

@lukaszwawrzyk
Copy link
Contributor Author

the loss of incrementality for Java-only modules

Maybe it wasn't stated clear enough, but the fix it is related to is actually totally separate thing. Probably it would be clearer if I had created separate PRs. Incrementality never worked with java modules without scala library in dependencies, with or without compile-to-jar. If it is fixed properly, it will work with compile-to-jar as well.

potential slowdown of incremental compilation

not sure why would that be the case.

the need for copying of the JARs

for runtime, yes, that is a drawback

I wonder whether the (unknown) speedup of the initial JAR downloading (only when using CachedCompilationProvider) really outweighs all the drawbacks, and whether there is a different way to speedup the initial downloading & extractions (e.g., as described above).

The speedup for the case of "building" 500+ modules via downloading cache is known. For the original integration it was from 9-10 mins down to 29-40 minutes (windows is really unstable in IO), tested with 3 runs. With fixes from this PR, and some other internal fixes we went down to 5 minutes now. The class unzipping was the bottleneck. There were more tests, differences could be even bigger. Windows is really terrible at creating multiple files.
To answer the question, for us it outweighs the drawbacks (actually for our consideration, there is only one relevant drawback: copying jars). Such low time to setup a workspace after a pull compared to what was before is a really big change.

For all that, the effectiveness of the scheme as such is out of scope of this PR. If you find the scheme useful, so be it. But we do need to make sure that the modification is reasonable and cohesive (yet this doesn't mean that everything should be 100% perfect and compatible with every single feature of IDEA -- some reasonable compromises are totally OK).

A feature that is not enable by default, but can be enabled via the standard UI is not considered "experimental" (at least, unless explicitly stated so). Besides, some modifications affect the non-optional mechanisms. So, we still need to be careful and don't break anything unnecessarily.

We could alter the checkbox label to Compile to jar (experimental). I am not sure which modifications affect the non-optional mechanisms though.

@lukaszwawrzyk
Copy link
Contributor Author

To sum this up: I would appreciate if you could list the commits with changes that you would be willing to merge as they are. I would create a PR for them.

Then probably a PR for ignored scalac options if I know where to put the settings for it.

Finally I would put a pr with compile to jar, that will probably require more polishing.

@pavelfatin
Copy link
Collaborator

pavelfatin commented Mar 13, 2019

Thanks for the answers.

Here's a possible roadmap:

  • Use fine-grained PRs.
  • Store the JARs inside the output directories. In this way the "directory" parts in the IDEA UI or in the SBT project configuration will be technically correct. This will also prevent the output JARs from being indexed and re-indexed by IDEA. And this will make the "Clean output directories on rebuild" work as expected.
  • Instead of the Zinc settings page, add checkbox after the "Incrementality type" when the value is "Zinc", name the checkbox "Compile to JAR*" (with an asterisk) and display a tooltip that says that the mode is experimental, why one may want to use that (not for speed, not to pack the files), and what are the known quirks / drawbacks, files layout, maybe a hyperlink, etc.
  • Add a "Compilation" tab to the Settings / Languages / Scala and put the excluded compiler option there (the setting should probably be application-wide, not project-wide). That's a trade-off, because that place is not near the compiler settings, but, on the other hand, 99.9% of the time people doesn't need to touch those settings, so it's also an advantage (in terms of UX).
    We then need to support the excluded settings for the IDEA compilation mode as well. - either you might do that, or you may create a YouTrack ticket and we will do that ourselves.
  • Arguments refactoring is OK (even though the effect is questionable), but we should probably omit the compression, unless there are factual data that supports its positive, rather than negative effect.
  • The compilation of Java-only modules using IDEA is OK (even though not ideal), but we need to mention that such modules are not compiled to JARs in the tooltip. And we may add a code comment that explains the reason for-, and pros & cons of the current solution (and that more research is needed).

I. e. we're OK with most of the updates, albeit with some polishing.

@pavelfatin
Copy link
Collaborator

pavelfatin commented Mar 13, 2019

As for the compile-to-JAR thing:

I rather meant that improvement can be observed. .... This can be in order of seconds.

A typical user shouldn't use this mode to speedup the compilation. We should definitely explain that in the description.

The requirement to unzip all classes vs just keep jared classes is the significant part here that makes all the difference.

It would be interesting to know the absolute numbers for a single JAR, i.e. "download & extract (in parallel)" vs "just download".

We download 1 jar per output in ctj case, and that's about it. For .class files, jar is extracted directly from remote location to the output directory.

So, you extract from the stream, without saving the JAR to disk? Do you download the JARs in parallel or sequentially?

I am not sure if I understand. What is the FS caching used for and what problems can it cause?

It takes time to read the archive metadata, and, in contrast to the native file system metadata, that data is not cached.

As far as I understand this is handled by zinc, or rather scalac itself. The class files of dependencies will be passed as jars instead of directories.

It doesn't matter which part needs to read the data. If the scalac needs to do that, than the scalac needs to read the 10 archives.

Any caching is handled in scalac, and scalac is quite good with using jars.

Maybe (though, not necessarily jars that constantly change, and not for output), and yet it's better to benchmark that.

We didn't identify any slow down on that.

You might not notice that on batch compilation - you need to test specific use cases, e.g. when there are 10 files in 10 JARs on incremenal compilation of 1 file - the difference might be huge. Also note that scalac simply cannot cache the JAR contents if there's no persistent compile server.

It is not a loss. We can't lose something we don't have.

That is, comparing to the "ideal" implementation :)

I am not sure what do you mean by "it seems that request merging won't increase the I/O in any meaningful way, and so is unnecessary," By request merging you mean merging arguments into one? It reduces number of send calls to socket api.

Reducing the number of socket calls (from 2 to 1?) won't increase the spead in any meaningful (or even observable) way.

The speedup for the case of "building" 500+ modules via downloading cache is known. For the original integration it was from 9-10 mins down to 29-40 minutes (windows is really unstable in IO), tested with 3 runs. With fixes from this PR, and some other internal fixes we went down to 5 minutes now.

So, the speedup is from 9-10 mins to 5 mins? That's not bad (but also not exactly a game-changer, given that downloading is only needed once, initially).

Is it on Windows? What are the numbers on Linux? Also, what is the effect of omitting the JAR extraction specifically (apart from those other fixes)?

Windows is really terrible at creating multiple files.

Sure, I can believe that. But the JARs need to be downloaded over the network, and it's harder to believe that the extraction is ~twice as slow as the downloading, if the process is performed in the optimal way (that is, extraction from the stream, without saving to disk, sequentially).

Also note that the numbers may vary depending on the network speed / use, the particular hardware (CPU / SSD), and on whether the JARs are compressed.

(all that is not related to the PR per se - I'm just wondering whether it's possible to improve / simplify the scheme)

@pavelfatin
Copy link
Collaborator

pavelfatin commented Mar 13, 2019

The thing is rather curious :) Here's a benchmark:

import java.io._
import java.nio.file.{Files, Paths, StandardCopyOption}
import java.util.jar.JarInputStream

object Benchmark {

  def main(args: Array[String]): Unit = {
    val file = new File("/home/user/idea.jar")
    val input = new JarInputStream(new ByteArrayInputStream(Files.readAllBytes(file.toPath)))
    val size = file.length.toDouble / 1024 / 1024

    println(s"Size: ${size.toInt} MiB")

    val begin = System.currentTimeMillis

    while (true) {
      val entry = input.getNextJarEntry
      if (entry == null) {
        val elapsed = System.currentTimeMillis - begin
        println(s"Elapsed: $elapsed ms")
        println(s"Speed: ${size / elapsed * 1000} MiB/s")
        return
      }
      if (!entry.isDirectory) {
        val path = Paths.get("/home/user/temp", entry.getName)
        val directory = path.getParent
        if (!directory.toFile.exists) {
          Files.createDirectories(directory)
        }
        Files.copy(input, path, StandardCopyOption.REPLACE_EXISTING)
      }
    }
  }
}

The processing speed of the idea.jar (on MPB 15 2015 & Linux) is ~60 MiB/s, or ~500 Mbit/s - that's a lot, so for a 2x speedup, the JAR must be downloaded at ~1 Gbit/s (though, the benchmark is not on Windows).

It would be interesting if you measure the download speed and the JAR processing speed in your typical setup.

BTW, if download & processing is >> max(download, processing), then you may consider performing the two operations asynchronously, with a RAM buffer (because, in principle, there might be some locking between the input / output buffers, for example, see "Parallel copying" mode in https://plugring.farmanager.com/plugin.php?pid=333 - it actually provides ~2x speedup when using different physical drives).

@pavelfatin
Copy link
Collaborator

It seems that, at this point, we may just amend this PR, since we have already analyzed / discussed "everything at once" anyway :)

@pavelfatin
Copy link
Collaborator

With regards to the possible incremental compilation slowdown: see the
Extracting zip archives in mc is horribly slow "bug-o-feature" of Midnight Commander.

What 7za x (on my machine) does in ~0.5 s, mc does in ~9 minutes - that's a ~1000x slowdown (!). This might or might not be an issue in the particular case, but it's definitely something worth checking.

@pavelfatin
Copy link
Collaborator

pavelfatin commented Mar 14, 2019

It seems that by performing the reading / writing asynchronously, it's possible extract a JAR almost 5x faster (~300 MiB/s vs ~60 MiB/s with the synchronous implementation).

Here's the benchmark:

import java.io._
import java.nio.file.{Files, Paths, StandardCopyOption}
import java.util.concurrent._
import java.util.jar.JarInputStream

object Benchmark {
  val InputFile = "/opt/idea-IC-183.5429.30/lib/idea.jar"
  val OutputDirectory = "/home/user/temp"

  def main(args: Array[String]): Unit = {
    for (_ <- 1 to 5) {
      benchmark()
      println()
      Thread.sleep(1000)
    }
  }

  private def benchmark(): Unit = {
    val service = Executors.newFixedThreadPool(Runtime.getRuntime.availableProcessors)

    val file = new File(InputFile)
    val size = file.length.toDouble / (1024 * 1024)
    println(f"Size: $size%.1f MiB")

    val inputStream = new JarInputStream(new ByteArrayInputStream(Files.readAllBytes(file.toPath)))

    val begin = System.nanoTime

    Iterator.continually(inputStream.getNextEntry).takeWhile(_ != null).filterNot(_.isDirectory).foreach { entry =>
      val entryData = new Array[Byte](entry.getSize.toInt)
      inputStream.read(entryData)

      service.execute { () =>
        val path = Paths.get(OutputDirectory, entry.getName)
        val directory = path.getParent
        if (!directory.toFile.exists) {
          Files.createDirectories(directory)
        }
        Files.copy(new ByteArrayInputStream(entryData), path, StandardCopyOption.REPLACE_EXISTING)
      }
    }

    service.shutdown()
    service.awaitTermination(1, TimeUnit.MINUTES)

    val elapsed = System.nanoTime - begin
    println(f"Elapsed: ${elapsed / 1e6}%.1f ms")
    println(f"Speed: ${size / (elapsed / 1e9)}%.1f MiB/s")
  }
}

In such a case, the JAR must be downloaded at ~2.5 Gbit/s to match the extraction speed :)

@retronym
Copy link
Contributor

retronym commented Apr 6, 2020

Relatedly in https://youtrack.jetbrains.com/issue/SCL-17305 I suggest that NailGun's security manager be disabled in the Scala Compile Server which reduces the cost of some of the File IO operations. Currently, every read creates a FilePermission object which internally canonicalizes the path, which seems particularly slow on Windows.

@pavelfatin
Copy link
Collaborator

@retronym We're considering not using Nailgun at all for that. Nailgun provides a standard protocol for running arbitrary programs in a warm-up JVM. We, however, need to run a specific set of commands, and we're the only "client". This will allow us to reduce the number of dependencies, improve security, let us use async-IO, protobuf, and socket / thread pools more effectively.

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.

7 participants