Improve Maven benchmarking and add profiling support #571
Conversation
lptr
changed the title
lptr
force-pushed
the
lptr/maven-becnhmarking-and-profiling
branch
4 times, most recently
from
0b5130e
to
f6c87da
Compare
lptr
force-pushed
the
lptr/maven-becnhmarking-and-profiling
branch
from
f6c87da
to
3b425f3
Compare
| SnapshotCapturingProfilerController controller; | ||
| // TODO This only works with Async profiler, since the only thing we call from the controller is stopSession() | ||
| // Capture this in the type hierarchy somehow | ||
| if (settings.getProfiler() instanceof InstrumentingProfiler) { |
There was a problem hiding this comment.
🤔 Should we throw exception if profiler is not instance of InstrumentingProfiler?
Another idea:
Maybe a profiler would have something like:
Profiler.supports(BuildTool)
There was a problem hiding this comment.
I tried that, but it got a bit ugly. I think the answer is to remodel the relationship between profilers and build tools on a bit deeper level, but it's a more impactful change than I think will fit in this PR.
| /** | ||
| * Whether this profiler supports only Gradle builds. | ||
| */ | ||
| public abstract boolean requiresGradle(); |
There was a problem hiding this comment.
🤔 It's a bit unfortunate we have this boolean requiresGradle(), maybe we could have some more generic method.
There was a problem hiding this comment.
Yes, 100%. Again, let's discuss how we want to model this relationship.
There was a problem hiding this comment.
Essentially, 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.
| // TODO This only works with Async profiler, since the only thing we call from the controller is stopSession() | ||
| // Capture this in the type hierarchy somehow | ||
| if (settings.getProfiler() instanceof InstrumentingProfiler) { | ||
| InstrumentingProfiler profiler = (InstrumentingProfiler) settings.getProfiler(); |
There was a problem hiding this comment.
Oh I guess because of profiler.newSnapshottingController`, I wonder how we could make that differently without a cast 🤔
There was a problem hiding this comment.
This is what I dislike about this, yes. I think we should re-model how profilers and build tools work together, but that's outside the scope of this PR.
Builds on: