RFR: 8330198: Add some class loading related perf counters to measure VM startup [v8]

[Calvin Cheung]

On Mon, 10 Jun 2024 07:44:21 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   remove Arguments::perf_class_link()
>
> src/hotspot/share/interpreter/linkResolver.cpp line 1735:
> 
>> 1733: 
>> 1734:   PerfTraceTimedEvent timer(ClassLoader::perf_resolve_invokehandle_time(),
>> 1735:                             ClassLoader::perf_resolve_invokehandle_count());
> 
> What does this do when the counters are not enabled?

It should just return. The following code in perfData.hpp.


    inline PerfTraceTimedEvent(PerfLongCounter* timerp, PerfLongCounter* eventp): PerfTraceTime(timerp), _eventp(eventp) {
      if (!UsePerfData || timerp == nullptr) { return; }

> src/hotspot/share/runtime/java.cpp line 160:
> 
>> 158: }
>> 159: 
>> 160: void log_vm_stats(outputStream *st) {
> 
> I assume this generic name is because in the future it will print a lot more VM stats?

Yes.

> src/hotspot/share/runtime/java.cpp line 164:
> 
>> 162:   if (log.is_enabled()) {
>> 163:     ClassLoader::print_counters(st);
>> 164:   }
> 
> Probably worth adding a comment here as to why we actually print to the passed in stream and not the log stream., given we check if the log stream is enabled. Someone could easily think this is a typo/bug.

I can change `log_vm_stats` to accept a `bool` argument so that the the `st` becomes clear.


void log_vm_stats(bool use_tty) {
  LogStreamHandle(Info, perf, class, link) log;
  if (log.is_enabled()) {
    outputStream* st = use_tty ? tty : &log;
    ClassLoader::print_counters(st);
  }
}

> src/hotspot/share/runtime/threads.cpp line 835:
> 
>> 833:     log.print_cr("At VM initialization completion:");
>> 834:     log_vm_stats(&log);
>> 835:   }
> 
> If we are going to have more types of VM stats in the future, it is not clear how you will change this if-condition? Nor what stream you would pass in. ???

The if-condition could be something like:


if (log_is_enabled(Info, perf, class, link) ||
     log_is_enabled(Info, perf, xxx, yyy) ||
     ...)


Regarding which stream to pass in, with my proposed change in `log_vm_stats` above, the current fix would look like when calling from threads.cpp:
`log_vm_stats(false /* use_tty */);`
when calling from java.cpp:
`log_vm_stats(true /* use_tty */);`

Or do you prefer not having the `log_vm_stats` function and calling `ClassLoader::print_counters` directly?
If so, we don't need the compound `if` conditions in the above.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18790#discussion_r1633995004
PR Review Comment: https://git.openjdk.org/jdk/pull/18790#discussion_r1633995051
PR Review Comment: https://git.openjdk.org/jdk/pull/18790#discussion_r1633995194
PR Review Comment: https://git.openjdk.org/jdk/pull/18790#discussion_r1633995900