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

Zinc compiler enhancements #707

Closed
wants to merge 1 commit into from
Closed

Zinc compiler enhancements #707

wants to merge 1 commit into from

Conversation

fkorotkov
Copy link

@fkorotkov fkorotkov commented Oct 13, 2016

Take number 3 after #666 and #671 on fixing timeout while locking Zinc cache.

Background

See #666 and #671 for details.

TLDR: We have a project with 450+ project where ~100 of them in Scala. On a cold compilation where no caches are presented and we are compiling with max parallelism we see TimeoutExceptions while waiting for a zinc lock.

Solution

I've refactored scala compilation to use direct in-memory java compiler and use separate caches for different Zinc versions.

But most importantly I've refactored how the lock is used. Before it was acquired even on checking
local caches. Also compilation of sbt compiler interface was wrapped in the lock which led to the timeout. Now this compilation will be take place in a temp folder and a lock will be acquired only on coping the compiler interface to the cache folder.

Testing

I've ran v3.1.0 with this fix on our repo multiple times successfully. In our case it also dropped the overall compilation time by ~10%.

to: @ghale
cc: @eriwen @bmuschko

Use direct java compiler and create separate caches for different Zinc versions. Also refactored creating a new compiler so it acquires a lock only if needed and only for writing the final compiler interface to the cache.

Before a lock was acquired even for checking file existence in the cache and more importantly *compiling* the interface which affects performance of highly parallel builds.
@fkorotkov
Copy link
Author

Could someone PTAL? /cc @ghale @eriwen @bmuschko

@eriwen
Copy link
Contributor

eriwen commented Oct 17, 2016

Hi again Fedor. Thanks again for continuing to work at this troubling issue.

I've been looking at this very problem separately and will respond to you later this week. Assigning myself this review.

@eriwen eriwen self-assigned this Oct 17, 2016
@eriwen eriwen added this to the 3.3 milestone Oct 17, 2016
@eriwen eriwen changed the title Zinc compiler enchasments Zinc compiler enhacements Oct 19, 2016
@eriwen
Copy link
Contributor

eriwen commented Oct 20, 2016

Hi @fkorotkov, I had an initial look through this. I have pushed a few minor changes to another branch 32dd734 (if you allow edits from maintainers then I'm happy to add changes there).

I and have one thing to follow up on: Understand the implications of using in-memory Java compiler instead of forking

Meanwhile ScalaCompileParallelIntegrationTest is now failing (because the interface JAR is no longer where we expect). You can run it with ./gradlew :languageScala:integTest --tests "*ScalaCompileParallelIntegrationTest" - would you mind taking a look at it?

@fkorotkov
Copy link
Author

@eriwen sure thing! Will fix probably tomorrow 👍

eriwen added a commit that referenced this pull request Oct 21, 2016
File name started with compiler-interface-compiler-interface-...
Fixed associated ScalaCompileParallelIntegrationTest

Issue: #707, #733, #734
@eriwen
Copy link
Contributor

eriwen commented Oct 21, 2016

@fkorotkov I've got the tests fixed and made a couple minor tweaks. Doing one final pass with my mates and we'll probably get this merged very soon :)

@fkorotkov
Copy link
Author

@eriwen awesome news! thanks a lot!

eriwen added a commit that referenced this pull request Oct 22, 2016
- Acquire lock when trying to get pre-existing zinc interface JAR.
This prevents returning a partially-copied JAR.
- Create temp file under zinc cache dir so we can verify that no
files are left behind after creating compiler interface JAR.
- Log interface compiliation duration while debugging.

Issue: #707, #733, #734
eriwen added a commit that referenced this pull request Oct 23, 2016
This allows us to more easily verify that no files are leaked
from generating zinc compiler interface JAR.

Wrap Zinc cache closure in try/finally to avoid leaving it open
if an exception is thrown in operations before it's closed.

Print warning if compiler interface JAR takes over 30 seconds to
generate.

Issue: #707, #733, #734
eriwen added a commit that referenced this pull request Oct 23, 2016
- Moved ZincCompilerServices to be private inner class
- Fixed failures in ScalaCompileParallelIntegrationTest
- Fixed typo "coping" => "copying"
- Update Zinc compiler interface JAR name. File name started
  with compiler-interface-compiler-interface-
- Avoid leaking files when generating Zinc interface JAR
- Acquire lock when trying to get pre-existing zinc interface JAR.
This prevents returning a partially-copied JAR.
- Create temp file under zinc cache dir so we can verify that no
files are left behind after creating compiler interface JAR.
- Wrap Zinc cache closure in try/finally to avoid leaving it open
if an exception is thrown in operations before it's closed.
- Print warning if compiler interface JAR takes over 30 seconds to
generate.

Issue: #707, #733, #734
@eriwen
Copy link
Contributor

eriwen commented Oct 23, 2016

This was merged with bc8fe2f with a bit of cleanup in 92cd7a6.

Huge thanks @fkorotkov!

@eriwen eriwen closed this Oct 23, 2016
@eriwen
Copy link
Contributor

eriwen commented Oct 23, 2016

@fkorotkov I'm afraid I had to revert back to forking a JVM for Zinc interface compilation (799aa60). This caused JAVA_HOME to not be detected correctly. (You can reproduce this by running `./gradlew :scala:check --tests "*JreJavaHomeScalaIntegrationTest"

Perhaps this is due to the way Zinc looks for javac. Would you have any ideas?

@fkorotkov
Copy link
Author

@eriwen The in-memory one should use the same as the working process. I'm PTO this week and will take a look next week.

@fkorotkov
Copy link
Author

@eriwen I've reverted it back to direct one and ./gradlew :scala:integTest --tests "*JreJavaHomeScalaIntegrationTest" is working for me on Mac. What kind of failure do you see? Is it the windows only test case?

@fkorotkov
Copy link
Author

@eriwen any updates on how can I reproduce locally?

@eriwen eriwen changed the title Zinc compiler enhacements Zinc compiler enhancements Oct 31, 2016
@ov7a ov7a removed this from the 3.3 RC1 milestone Mar 28, 2024
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.

None yet

3 participants