RFR: 7903696: JMH: Add xctrace-based perfasm profiler for macOS [v3]

[Aleksey Shipilev]

On Sun, 7 Apr 2024 18:22:34 GMT, Filipp Zhinkin <fzhinkin at openjdk.org> wrote:

>> Currently, the only perfasm profiler available on macOS is dtraceasm.
>> While it serves its purposes well, there are a few shortcomings: dtrace requires superuser privileges and it can only sample by timer interrupts.
>> 
>> Xcode comes with a performance-analysis tool called Instruments and it has a command-line tool, called xctrace.
>> xctrace allows profiling an app using both timer interrupts and PMI, and it does not require superuser privileges.
>> 
>> This PR brings JMH a profiler that uses xctrace on macOS.
>> 
>> ~~There are two profilers:~~
>> ~~- XCTraceAsmProfiler, a perfasm profiler based on xctrace that works fine out of the box;~~
>> ~~- XCTraceNormProfiler, a perfnorm profiler that requires mandatory parameter, that has to be setup using Instruments UI.~~
>> 
>> ~~The latter is added as a PoC and could be wiped out of this PR.~~
>> 
>> Unfortunately, `xctrace` exports data in XML format, thus most of the code is responsible for XML parsing.
>
> Filipp Zhinkin has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - 7903696: Removed an option unsupported by older xctrace version
>  - 7903696: Support legacy backtrace format
>  - 7903696: Detect xctrace using xcode-select

This is an impressive chunk of work, thank you!

It seems to be working on my M1 quite well. I need to try more things with it.

I have some stylistic comments.

jmh-core/src/main/java/org/openjdk/jmh/profile/XCTraceAsmProfiler.java line 86:

> 84: 
> 85:     private long recordStartMs = 0;
> 86:     private long forkStartTimeMs = 0;

No need for `= 0` initialization here, these are default values anyway.

jmh-core/src/main/java/org/openjdk/jmh/profile/XCTraceAsmProfiler.java line 109:

> 107:                 .ofType(String.class)
> 108:                 .defaultsTo("Time Profiler");
> 109:         correctOpt = parser.accepts("fixStartTime", "Fix the start time by the time it took to launch.")

Newline between these two, please.

jmh-core/src/main/java/org/openjdk/jmh/profile/XCTraceAsmProfiler.java line 157:

> 155:             return super.afterTrial(br, pid, stdOut, stdErr);
> 156:         } finally {
> 157:             XCTraceSupport.removeDirectory(temporaryDirectory);

Do you really need `finally` here? Can we just record the result from super and then do `removeDirectory`?

jmh-core/src/main/java/org/openjdk/jmh/profile/XCTraceTableOfContentsHandler.java line 68:

> 66:         } else if (schema.equals(ProfilingTableType.COUNTERS_PROFILE.tableName)) {
> 67:             parseCountersProfile(attributes);
> 68:         }

Optional: You can use switch over Strings here?

jmh-core/src/main/java/org/openjdk/jmh/profile/XCTraceTableProfileHandler.java line 70:

> 68:     private final Consumer<XCTraceSample> callback;
> 69: 
> 70:     private XCTraceSample currentSample = null;

No need for `null` initialization.

jmh-core/src/main/java/org/openjdk/jmh/profile/XCTraceTableProfileHandler.java line 93:

> 91:     private static long parseAddress(Attributes attributes) {
> 92:         String val = attributes.getValue(XCTraceTableHandler.ADDRESS);
> 93:         if (!val.startsWith("0x")) throw new IllegalStateException("Unexpected address format: " + val);

Braces and new line, please.

jmh-core/src/main/java/org/openjdk/jmh/profile/XCTraceTableProfileHandler.java line 288:

> 286: 
> 287:     private static abstract class TraceElement {
> 288:         public final long id;

Here and later in `TraceElement` subclasses. For extra sanity, can you turn these fields `private` and expose proper (immutable) getters for them?

jmh-core/src/main/java/org/openjdk/jmh/profile/XCTraceTableProfileHandler.java line 309:

> 307: 
> 308:     private static final class LongHolder extends TraceElement {
> 309:         public long value = 0;

Style: no need for inits.

-------------

PR Review: https://git.openjdk.org/jmh/pull/130#pullrequestreview-2010794311
PR Review Comment: https://git.openjdk.org/jmh/pull/130#discussion_r1571994451
PR Review Comment: https://git.openjdk.org/jmh/pull/130#discussion_r1571995093
PR Review Comment: https://git.openjdk.org/jmh/pull/130#discussion_r1572685549
PR Review Comment: https://git.openjdk.org/jmh/pull/130#discussion_r1572702103
PR Review Comment: https://git.openjdk.org/jmh/pull/130#discussion_r1572702919
PR Review Comment: https://git.openjdk.org/jmh/pull/130#discussion_r1572703334
PR Review Comment: https://git.openjdk.org/jmh/pull/130#discussion_r1572707098
PR Review Comment: https://git.openjdk.org/jmh/pull/130#discussion_r1572707639