Reapply the zinc upgrade in 1.6.0.dev #4477
Comments
|
Additionally, there have been two different user-reported errors in incremental compilation: sbt/zinc#292 and one from @agenticarus. |
|
I repro the incremental compile slowdown... trying to isolate it. EDIT: I repro'd it on the first local build, but not on subsequent local builds... hmm. |
|
Not sure it's definitive yet, but I'm seeing 100% invalidation in this example (ie, zinc is choosing to recompile all of the files in the targets). Will go ahead and compare that to master shortly. EDIT: Confirmed: for the same case, master compiles exactly one file out of about 600, which takes 2 minutes, vs 4-8m to compile all of them. |
|
Will be off this for the week, but am dumping some notes: |
stuhood
changed the title
### Problem The zinc 1.0.0-X7 upgrade resulted in a few issues, including complete target invalidation (pantsbuild#4477), and some unconfirmed assertion failures from within zinc. ### Solution This reverts pantsbuild#4419 until after the `1.3.0` stable branch has been cut. It should be re-landed immediately afterward.
### Problem The zinc 1.0.0-X7 upgrade resulted in a few issues, including complete target invalidation (pantsbuild#4477), and some unconfirmed assertion failures from within zinc. ### Solution This reverts pantsbuild#4419 until after the `1.3.0` stable branch has been cut. It should be re-landed immediately afterward.
|
The commit that disabled this was 325771d. At the risk of introducing additional bugs during the upgrade, I'm going to update to zinc |
|
For reference: sbt/zinc#304 |
|
Made a bit more progress here. With a workaround for the |
|
Made progress toward benchmarking incremental performance here, but my logging hacks mean that I'm not currently getting debug level logging out. Need to un-hack a few things. Pushed the branch over here: master...twitter:stuhood/zinc-upgrade-2017-06 |
|
Alright, I've been able to get enough information to confirm that we're still getting full invalidation as described on April 17th. Will begin debugging tomorrow. |
|
So, it looks like the analysis callback is not being used (??) for scala compilation in the branch, so only java gets any relations recorded. So, for scala I always see: But it is being used for Java. so Relations/Stamps/"analysis in general" is being recorded for Java. Looking more. Also reached out to Jorge to see whether there is something obviously wrong. |
|
Argh. Figured out what this was. See sbt/zinc#317 (comment) for a summary. In the meantime, I ended up implementing #4513, and that also seems to be working in local tests. The summary of TODOs for this is:
|
|
Strategy for item (2) above is roughly:
Both the |
@stuhood @baroquebobcat Have you been able to reproduce this? Feel free to mention me if there's some news that may interest me 😄. With the new analysis read/write backend and the RC, I hope that the nastiest parts of the Zinc integration in Pants will disappear. |
Uncertainty around |
This is my main focus on the changes around reading/writing analysis. This task is exposing all the bits that need to be changed for machine independence, so yeah, my plan is to implement the logic in Zinc directly rather than requiring you to rewrite it. One question: have you ever benchmarked how much it takes to rewrite the analysis files? |
### Problem Pants' zinc wrapper tool is currently configured directly on the `compile.zinc` task, which would cause duplication if some other task wanted to use the zinc install (see #4477 in particular). ### Solution Extract a `zinc` subsystem which is required by the `compile.zinc` task, and which can be consumed from other tasks in the future. ### Result Because the tool key is the same, only the options which allow for re-configuring the tool location needed to be deprecated: if they were overriding tool locations, users will need to move them from `--compile-zinc-zinc` to `--zinc-zinc`, etc.
|
Alright, the two halves of this change are out at #4728 and #4729. Landing them is blocked on sbt/zinc#325 and on the analysis portability changes that @jvican is working on. Thanks @jvican ! |
|
Incorporated zinc 1.0.0-RC3 and will test it internally early next week. As it stands, it looks like there is an issue in #4729 causing compiler plugin deps to not be included in all cases. EDIT: This is probably related to the option relocating that I did. |
|
I'm testing |
|
As far as I can tell, this is not a memory leak: it looks like overhead just increased (probably due to name hashing, which we were not using before). Unfortunately, a few of our largest targets no longer compile with our configured 6G of ram, so I'm going to have to see whether we can reduce our overhead in other ways... failing that, I might (temporarily?) increase our heap sizes, although that is a very costly proposition. I ended up including #4744, and it does seem to significantly decrease the amount of classloading (although it does not seem to affect memory usage). |
|
Hey Stu, could you please provide me a heap dump (or even better, a full profile dump) so that I can further investigate? I'm optimistic this overhead can be decreased, though it's true that Zinc is now doing more work. I have in my todo list several ideas to speed Zinc up and reduce % of total incremental compilation, but would like first to see if the increase you're seeing here has an easy fix. |
|
Alright, it looks like the memory pressure is not the largest issue: some code changes droppped the dependency count of the target I'm testing with by about 300, which removes most of the memory pressure. The memory pressure primarily affected cold compiles, so with that out of the way I'm back to investigating incremental compiles. The new issue I'm seeing is a huge amount of CPU time in loading analysis (on the order of 260 seconds loading analysis for 1400 targets). I'm currently working to confirm that we are hitting the analysis cache that our wrapper sets up (rather than maybe churning out analysis and loading it repeatedly). EDIT: This was because our analysis cache size was much too low. On the other hand, I think it has been too low for a while, so I suspect that missing the cache and being forced to load analysis is now much more expensive than it used to be. |
|
I have a new issue in Java compilation that I'm going to try to extract to an issue tomorrow: https://gist.github.com/stuhood/1f283e5b5703c931a8bf21e5e4b55f4e ... most likely a zinc 1.0.0 blocker. EDIT: Minimized this to sbt/zinc#389 |
|
#4807 fixes a few issues on the JVM side, but I just determined that the Additionally, Twitter won't be able to consume this version of zinc until sbt/zinc#389 is fixed. Finally, as described on #4807, we'll need to adjust how/when we pass the Unfortunately I'm getting ready to go on vacation, and so unless both of those issues get fixed before Monday morning, landing #4729 will have to wait until I return. |
### Problem While working through #4477, it became obvious that: 1. the performance achieved when the analysis cache is missed is pretty abysmal 2. a `log4j` JMX error was being logged during startup 3. the `forkJava` and `javaHome` settings are redundant: the only reason to pass `javaHome` would be to trigger forking Java 4. a large amount of unnecessary classloading was happening due to #4744 ### Solution 1. Set the default `zinc.analysis.cache.limit` value to `Int.MaxValue`, which will allow users who want to cap the size to set it lower, but otherwise not lead to performance cliffs when a target has more dependencies than the current limit. 2. Implicitly disable log4j JMX usage by setting the `log4j2.disable.jmx` property if it is not already set. 3. Remove the `forkJava` setting, to prepare to use the `javaHome` setting explicitly in #4729 4. Enable usage of `ClassLoaderCache` ### Result Fixes #4744, and removes a few more blockers for #4729.
|
Fixed a few more tests in #4729, but ran into sbt/zinc#451. |
|
@stuhood Have you already figured it out? If you remember, we had a discussion in Gitter about this, and you confirmed us that requiring JDK8 would not be an issue. 😄 |
|
@jvican : Yea, I think it will be a non-issue. I had forgotten that we had discussed that a few months ago, and needed to page it back in. |
wisechengyi
changed the title
### Problem (this change depends on #4728) As described on #4728, we currently need internal knowledge of zinc analysis in order to make it portable, and to extract products that are used by other tasks. Since the zinc analysis format is changing significantly (moving to protobuf, gaining/losing fields, etc), we need to stop parsing it. ### Solution Incorporates the version of the zinc wrapper published in #4728, and uses it in a `analysis.zinc` task to generate the `product_deps_by_src` and `classes_by_source` products from a `zinc_analysis` product published by zinc. Expands the extracted `Zinc` subsystem from #4720 to encapsulate compile dependency calculation, and expose the `FingerprintStrategy` and `DependencyContext` that `compile.zinc` was using. ### Result Zinc analysis is now a black box from the perspective of the pants python code. Also, the `compile.jvm-dep-check` task moved to the `lint` goal, and gained the unused dependency checks that used to be inlined in `compile.zinc` (and which would have prevented separation of the analysis extraction task). Fixes #4477, fixes #4513, fixes #4185, fixes #5444, and hopefully fixes a few other odd incremental compilation bugs.
### Problem (this change depends on #4728) As described on #4728, we currently need internal knowledge of zinc analysis in order to make it portable, and to extract products that are used by other tasks. Since the zinc analysis format is changing significantly (moving to protobuf, gaining/losing fields, etc), we need to stop parsing it. ### Solution Incorporates the version of the zinc wrapper published in #4728, and uses it in a `analysis.zinc` task to generate the `product_deps_by_src` and `classes_by_source` products from a `zinc_analysis` product published by zinc. Expands the extracted `Zinc` subsystem from #4720 to encapsulate compile dependency calculation, and expose the `FingerprintStrategy` and `DependencyContext` that `compile.zinc` was using. ### Result Zinc analysis is now a black box from the perspective of the pants python code. Also, the `compile.jvm-dep-check` task moved to the `lint` goal, and gained the unused dependency checks that used to be inlined in `compile.zinc` (and which would have prevented separation of the analysis extraction task). Fixes #4477, fixes #4513, fixes #4185, fixes #5444, and hopefully fixes a few other odd incremental compilation bugs.
### Problem (this change depends on #4728) As described on #4728, we currently need internal knowledge of zinc analysis in order to make it portable, and to extract products that are used by other tasks. Since the zinc analysis format is changing significantly (moving to protobuf, gaining/losing fields, etc), we need to stop parsing it. ### Solution Incorporates the version of the zinc wrapper published in #4728, and uses it in a `analysis.zinc` task to generate the `product_deps_by_src` and `classes_by_source` products from a `zinc_analysis` product published by zinc. Expands the extracted `Zinc` subsystem from #4720 to encapsulate compile dependency calculation, and expose the `FingerprintStrategy` and `DependencyContext` that `compile.zinc` was using. ### Result Zinc analysis is now a black box from the perspective of the pants python code. Also, the `compile.jvm-dep-check` task moved to the `lint` goal, and gained the unused dependency checks that used to be inlined in `compile.zinc` (and which would have prevented separation of the analysis extraction task). Fixes #4477, fixes #4513, fixes #4185, fixes #5444, and hopefully fixes a few other odd incremental compilation bugs.
Testing internally, the new zinc release was much slower at doing incremental builds, ie 2x or more.
We should do some further testing and see if we can narrow down what the cause is and whether we should shoot for fixing it, leaving it in, or reverting it for 1.3.0.
The text was updated successfully, but these errors were encountered: