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

Reimplement parallelism internal logic #1991

Merged
merged 6 commits into from
Oct 17, 2019
Merged

Conversation

arturbosch
Copy link
Member

From the changelog:

- `parallel=true` and `--parallel` now effect both the compilation and analysis phase. 
- Users of Gradle's `--parallel` flag are encouraged to turn off the parallelism of detekt. Or turn it on otherwise.
- detekt does not use the `ForkJoinPool.commonPool()` anymore internally. When embedding detekt feel free to pass your own `ExecutionService` to the `ProcessingSettings`.

Benchmarks:
detekt's parallelism is notated as p=false|true, gradles parallelism is notated as --p=false|true

Gradle Plugin run on detekt's codebase, ~40k lines:
1.1 clean buid --p=false p=false | 2.38s, 2.27s
1.2 clean build --p=false p=false | 2.30s, 2.25s

1.1 clean build --p=true p=false | 1.55s, 1.46s
1.1 clean build --p=true p=true | 1.53s, 1.47s
1.2 clean build --p=true p=false | 1.48s, 1.46s
1.2 clean build --p=true p=true | 1.46s, 1.45s

CLI on detekt's codebase, ~40k lines:
1.1 p=false | ~4.14
1.2 p=false | ~4.17
1.1 p=true | ~2.85
1.2 p=true | ~2.80

Conclusion:
There are no real performance benefits for a middle large project like detekt.
However there still could be some benefits for really large multi module Gradle projects when using Gradle's --parallel and using detekt's parallel=false.
Imo the code looks cleaner now and there is the possibility now to just use one thread.
We also do not use the CommonThreadPool and do not make him starve.

Using the common thread pool may have lead to  starvation when embedding detekt.
Only the compilation of Kotlin files was done in parallel using this flag.
All analysis processing was always done in parallel.
@arturbosch arturbosch added this to the 1.2.0 milestone Oct 5, 2019
@codecov-io
Copy link

codecov-io commented Oct 5, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@a367813). Click here to learn what that means.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1991   +/-   ##
=========================================
  Coverage          ?   80.39%           
  Complexity        ?     1990           
=========================================
  Files             ?      329           
  Lines             ?     5610           
  Branches          ?     1025           
=========================================
  Hits              ?     4510           
  Misses            ?      554           
  Partials          ?      546
Impacted Files Coverage Δ Complexity Δ
...in/kotlin/io/gitlab/arturbosch/detekt/core/Junk.kt 76.92% <ø> (ø) 0 <0> (?)
.../kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt 92.59% <ø> (ø) 6 <0> (?)
...itlab/arturbosch/detekt/core/ProcessingSettings.kt 77.77% <100%> (ø) 16 <2> (?)
.../io/gitlab/arturbosch/detekt/cli/runners/Runner.kt 88.88% <100%> (ø) 5 <1> (?)
.../arturbosch/detekt/cli/runners/SingleRuleRunner.kt 86.11% <100%> (ø) 6 <0> (?)
...otlin/io/gitlab/arturbosch/detekt/core/TaskPool.kt 33.33% <33.33%> (ø) 3 <3> (?)
...io/gitlab/arturbosch/detekt/core/KtTreeCompiler.kt 48% <33.33%> (ø) 8 <3> (?)
...otlin/io/gitlab/arturbosch/detekt/core/Detektor.kt 50% <36.66%> (ø) 5 <3> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a367813...01d16dc. Read the comment docs.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Can we even test this with unit or integration tests?

arturbosch and others added 3 commits October 15, 2019 21:08
…Args.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
…etektor.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
@arturbosch arturbosch merged commit 65c791b into master Oct 17, 2019
@arturbosch arturbosch deleted the reimplement-parallel branch October 17, 2019 18:41
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
* Reimplement --parallel logic

Using the common thread pool may have lead to  starvation when embedding detekt.
Only the compilation of Kotlin files was done in parallel using this flag.
All analysis processing was always done in parallel.

* Update documentation for --parallel change

* Rely on stdlib use

* Update detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>

* Update detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Detektor.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
* Reimplement --parallel logic

Using the common thread pool may have lead to  starvation when embedding detekt.
Only the compilation of Kotlin files was done in parallel using this flag.
All analysis processing was always done in parallel.

* Update documentation for --parallel change

* Rely on stdlib use

* Update detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>

* Update detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Detektor.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
This pull request was closed.
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.

3 participants