Made AnalysisCallback thread-safe again. Re sbt/sbt#6198 #957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For the parallel Hydra Scala compiler to work with sbt and zinc, it's paramount that the AnalysisCallback is thread-safe. The bulk of the work was done in sbt#410, but the changes introduced to support pipelined compilation were not thread-safe, hence could potentially break compilation via Hydra. There are three categories of concurrency hazards that are fixed: * Memory visibility issues (`@volatile` helps with this) * Compiler/JVM optimizations leading to instructions re-ordering (`@volatile prevents this from happening in the sections of interest) * Race conditions due to concurrent read/write (`AtomicBoolean` CAS heals this) Mind that instead of `@volatile`, it was perfectly fine to use `AtomicReference`/`AtomicBoolean`, but I opted for `@volatile` in the places where CAS were not required. The rationale is to apply KISS.
|
\cc @dragos |
This was referenced Dec 20, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For the parallel Hydra Scala compiler to work with sbt and zinc, it's paramount
that the AnalysisCallback is thread-safe. The bulk of the work was done in
#410, but the changes introduced to support pipelined compilation were
not thread-safe, hence could potentially break compilation via Hydra.
There are three categories of concurrency hazards that are fixed:
@volatilehelps with this)prevents this from happening in the sections of interest)
AtomicBooleanCAS heals this)Mind that instead of
@volatile, it was perfectly fine to useAtomicReference/AtomicBoolean, but I opted for@volatilein the placeswhere CAS were not required. The rationale is to apply KISS.
I've tested this by building a local sbt with zinc produced out of this PR, and tested Hydra compilation in sbt 1.4 works. I don't believe there is a need for testing performance. The reason is that the performance impact of this PR is so irrelevant that is essentially impossible to observe.