RFR: CODETOOLS-7902928: Improve support for async-profiler 2.x [v2]
Aleksey Shipilev
shade at openjdk.java.net
Mon May 10 09:48:06 UTC 2021
On Fri, 7 May 2021 06:06:18 GMT, Jason Zaugg <jzaugg at openjdk.org> wrote:
>> Allow multiple events to be captured simultaneously in this version,
>> provided that JFR is chosen as output format.
>>
>> Delegate output file writing to the async-profiler. This is required
>> in 2.x for the JFR output but is supported in both versions. The file
>> path must be provided when starting the profiler with JFR output, so
>> we need to create the per-trial output directory in the first
>> `beforeIteration`.
>>
>> Avoid the character '%' in the generated directory name as this
>> is interpreted by async-profiler as part %p or %t placeholder.
>
> Jason Zaugg has updated the pull request incrementally with one additional commit since the last revision:
>
> Fixup: remove incorrect/unneeded 'reset' argument
This looks good from the first look, I'll do some trial runs now.
jmh-core/src/main/java/org/openjdk/jmh/profile/AsyncProfiler.java line 76:
> 74:
> 75: private boolean warmupStarted = false;
> 76: private boolean measurementStarted = false;
You can skip initialization to default value. In fact, I believe Sonar would complain to me about this later. You can also drop default initialization in the adjacent fields, while you are at it.
jmh-core/src/main/java/org/openjdk/jmh/profile/AsyncProfiler.java line 247:
> 245: if (version.startsWith("1.")) {
> 246: isVersion1x = true;
> 247: }
`isVersion1x = version.startsWith("1.")` to explicitly initialize the flag on all paths?
jmh-core/src/main/java/org/openjdk/jmh/profile/AsyncProfiler.java line 319:
> 317: break;
> 318: case jfr:
> 319: dump(trialOutDir, "%s.jfr", "jfr");
Say `// JFR is already dumped into file` in this `case`?
jmh-core/src/main/java/org/openjdk/jmh/profile/AsyncProfiler.java line 322:
> 320: // include % in the file name.
> 321: String fileName = benchmarkParams.id().replace("%", "_");
> 322: trialOutDir = new File(this.outDir, fileName);
Minor pre-existing nit: `this.` looks unnecessary, can be dropped?
jmh-core/src/main/java/org/openjdk/jmh/profile/AsyncProfiler.java line 341:
> 339: }
> 340: } catch (IOException e) {
> 341: e.printStackTrace();
Throw `new ProfilerException(e)` here?
-------------
PR: https://git.openjdk.java.net/jmh/pull/37
More information about the jmh-dev
mailing list