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