-
Notifications
You must be signed in to change notification settings - Fork 153
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
Some usability improvements #221
Conversation
…ommand-line from the `Profiler` implementations, so that multiple strategies can share a `Profiler` implementation.
…ngs to profile heap allocations using async-profiler. This allows a batch run that can capture CPU usage snapshots and heap allocation snapshots without having to run `gradle-profiler` twice. It also simplifies capturing heap allocation snapshots without the need to tweak the async-profiler settings.
…--yourkit-sampling` with `--profile yourkit-tracing` and `--yourkit-memory` with `--profile yourkit-memory`. This is to allow CPU usage and memory snapshots to be captured from a single `gradle-profiler` invocation.
…tains a '/' character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff!
Left a question regarding test coverage
/** | ||
* Represents some profiling strategy. Produces {@link Profiler} instances from a set of command-line options. | ||
*/ | ||
public abstract class ProfilerFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for splitting profiler factories out
given: | ||
instrumentedBuildScript() | ||
|
||
when: | ||
new Main().run("--project-dir", projectDir.absolutePath, "--output-dir", outputDir.absolutePath, "--gradle-version", minimalSupportedGradleVersion, "--profile", "async-profiler", "assemble") | ||
new Main().run("--project-dir", projectDir.absolutePath, "--output-dir", outputDir.absolutePath, "--gradle-version", latestSupportedGradleVersion, "--profile", "async-profiler", "assemble") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ shouldn't we test both minimalSupportedGradleVersion
and latestSupportedGradleVersion
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, probably. I figured other tests took care of covering the actual Gradle invocation stuff for all of the versions, and this test can assume that it works fine. I changed it to the latest version because this is more likely to be used with the async-profiler integration (rather than Gradle 3.3, which is the minimal version) and also so I didn't need to figure out how to run the tests with Java 8 (Gradle 3.3 doesn't work on Java 11).
I wonder if it would be better to keep which profiler to use and what to profile separately on the command line. Something like: > gradle-profiler --profiler async-profiler --profile heap
> gradle-profiler --profiler async-profiler --profile cpu # the default
> gradle-profiler --profiler yourkit --profile heap
> gradle-profiler --profiler jprofiler --profile heap WDYT? |
With the changes in this PR, I can do |
Ah, I didn't realize that you now can start multiple profiler runs with a single |
This PR is a bucket of minor improvements:
--profiler async-profiler-heap
. This is intended to provide some preset async-profiler configuration to capture heap allocation snapshots. It also allows both CPU and heap snapshots to be captured by a singlegradle-profiler
invocation.--profiler yourkit
measures CPU usage using sampling,--profiler yourkit-tracing
measure CPU usage using tracing, and--profiler yourkit-heap
measures heap allocation.