-
Notifications
You must be signed in to change notification settings - Fork 156
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
Improve Maven benchmarking and add profiling support, take 2 #576
Conversation
On modeling profilers vs build tools: There are profilers that can run via Java agent, and profilers that can run as an external process, and profilers that support both. And then there are build tools that support supplying an agent (Gradle, Maven), build tools that support a long running daemon (Gradle only), and build tools that support neither (Buck and Bazel might be these, at least as a start). I think this can be modeled nicely, and will make the compatibility between tool and profiler be obvious. |
Use it when it is available and a tool path is not specifically configured.
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.
Overall LGTM.
I added some questions to clarify changes. The one that I am most interested in is the one in MavenScenarioInvoker
, I wonder what happens with incompatible profilers with Maven
- `iterations`: Number of builds to actually measure | ||
- `warm-ups`: Number of warmups to perform before measurement | ||
- `jvm-args`: Sets or overrides the jvm arguments set by `org.gradle.jvmargs` in gradle.properties. | ||
|
||
### Profiling change handling | ||
|
||
How a build tool handles changes to the source code can have a significant impact on the performance of the build. | ||
Gradle Profiler can simulate different kinds of changes to the source code to measure the impact of these changes on the build performance. | ||
These changes are applied by mutators at different points in the build benchmark process. | ||
Some mutators execute at a specific point, others can be configured to execute at a specific point, specified by the `schedule` parameter: | ||
|
||
- `SCENARIO`: before the scenario is executed, | ||
- `CLEANUP`: before cleaning preceeding each build invocation, | ||
- `BUILD`: before the build invocation (after cleanup). | ||
|
||
#### Source code mutators | ||
|
||
These mutations are applied before each build, and they introduce different kinds of change to the source code. |
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.
Nice 👍
@@ -53,7 +59,7 @@ public File getToolHome() { | |||
|
|||
@Override | |||
public boolean createsMultipleProcesses() { | |||
return false; | |||
return true; |
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.
❓ What does this change do, does it have any consequence?
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.
This informs the profiler invocation part whether to expect a long running daemon or many small processes. It was not used for anything other than Gradle. We now say that other build tools will always start a new process per invocation.
/** | ||
* Whether this profiler supports only Gradle builds. | ||
*/ | ||
public abstract boolean requiresGradle(); |
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.
Comment on the method is:
Whether this profiler supports only Gradle builds.
Should we rename a method to: supportsOnlyGradle()
to better match the comment?
|
||
controller = profiler.newSnapshottingController(scenarioSettings); | ||
} else { | ||
profileEnvironment = ImmutableMap.of(); |
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.
❓ What happens in the case user sets incompatbile profiler in that case? Do we still run the build and return some result?
Should we throw exception here to warn a user that their setup is incorrect?
FTR, we agreed to merge this as is, and revisit how profilers are configured / matched with build tools soon. |
This is reopening #571.