RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]

Tue Jan 25 09:14:48 UTC 2022

On Mon, 24 Jan 2022 22:23:33 GMT, Tyler Steele <duke at openjdk.java.net> wrote:

>> Just in time for the holidays I have completed an implementation of the JFR functionality for AIX. As a side note, this is my first submission to OpenJDK 👋
>> 
>> ### Implementation notes and alternatives considered
>> 
>> After modifying the build system to allow the --enable-jvm-feature-jfr to work on AIX, my task was to implement the interfaces from os_perf.hpp. The os_perf_aix.cpp implementation that existed was, I believe, a copy of the Linux implementation. A review of the code in that file showed that NetworkInterface, CPUPerformanceInterface, and SystemProcessInterface would require modification to work on AIX. Using the Linux implementation as a guide, I initially expected to use files from the aix procfs like /proc/<pid>/psinfo, and /proc/<pid>/status in a manner similar to the Linux implementation. However, I ended up using libperfstat for all functionality required by the interfaces.
>> 
>> ### Testing
>> 
>> Testing for JFR seems to be quite light in the repo both before and after this change. In addition to performing manual testing, I have added some basic sanity checks that ensure events can be created and committed (using jtreg), and performs some basic checks on the results of the interface member functions (using gtest).
>> 
>> ### More notes
>> 
>> I've sent an email to the JFR group about a TOC overflow warning I encountered while building (for the release server target). I believe the fix is to pass -qpic=large when using the xlc toolchain, but my modifications to flags-cflags.m4 have not been successful in removing this warning.
>
> Tyler Steele has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
> 
>  - Merge branch 'master' into JDK-8203290
>  - Implements JFR on AIX
>    
>    - Implements interfaces from os_perf.hpp in os_perf_aix.cpp
>    - Updates libperfstat_aix to contain functionality needed by os_perf_aix
>    - Implements missing functionality in loadlib_aix (Fixes failure noted
>    by TestNativeLibraies.java)
>    - Removes platform checks for --enable-feature-jfr (Now enable-able on
>    all platforms)
>    - Enables TestNetworkUtilizationEvent.java which now passes on AIX
>    - Updates AIX JavaThread::pd_get_top_frame_for_profiling with changes from Linux

Hi Tyler,

nice work. This is a non-complete review of some things I spotted. Part of it is bikeshedding, feel free to ignore those.

Cheers, Thomas

src/hotspot/os/aix/libperfstat_aix.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2022, 2022, Oracle and/or its affiliates. All rights reserved.

Is there a reason for this copyright addition?

src/hotspot/os/aix/loadlib_aix.cpp line 375:

> 373: }
> 374: 
> 375: bool LoadedLibraries::copy_list(LoadedModuleList** head) {

I'd rename the function to something clearer like `reload_and_copy_list` since the reload may come as a surprise

src/hotspot/os/aix/loadlib_aix.hpp line 79:

> 77:   private:
> 78:     const loaded_module_t _module;
> 79:     const LoadedModuleList* _next;

While looking at this code, I realized how old it is (the copyright is wrong, this predates OpenJDK and was written ~2005). Today I would do some things differently, e.g. use a hash table for string interning.

About your change: `LoadedModuleList` is basically a duplication of the `entry_t`+`loaded_module_t` tupel. While I am not against it, it's duplication and a bit much boilerplate. You should at least redo the recursive deletion scheme. That will blow up if no tail call optimization happens.

I assume the whole copy list business is due to concurrent reloading? And because LoadedLibs does not offer an iteration interface?

If you feel up to it, I would simplify this:
- merge `entry_t` and `loaded_module_t` into one structure: give `loaded_module_t` a `next` pointer and get rid of entry_t. This is a scheme we use all over the hotspot (embedding list pointers directly into the payload structures) so it is fine and we save one indirection
- in LoadedLibs implementation, remove entry_t and use loaded_module_t directly
- in your new copy_libs, build up a list of copied loaded_module_t structures under lock protection like you do now with LoadedModuleList
- Do deletion in a loop, not recursively, with something like a new `static LoadedLibs::delete_list(loaded_module_t* list);`

Alternative: give LoadedLibs a callback-like functionality where you iterate the original list under lock protection and just call the given callback or closure class. In that closure, you call the original perfstat callback, so no need to pollute LoadedLibs with perfstat symbols.

Just as an info, in these files we tried to avoid JVM infrastructure, hence the plain `::malloc` instead of `os::malloc`  and we use none of the helper classes the JVM offers. E.g. instead of just using `GrowableHeapArray`, which would be nice and convenient. That is because using JVM infrastructure introduces dependencies to VM state and time (e.g. there are uninitialized time windows at startup), and we wanted this code to work as reliably as possible. Also should work during signal handling. Because the VM may crash at any point, and at any point we want to see call stacks and list of loaded libraries in the hs-err file.

src/hotspot/os/aix/os_perf_aix.cpp line 33:

> 31: #include "runtime/os_perf.hpp"
> 32: #include "runtime/vm_version.hpp"
> 33: #include "utilities/globalDefinitions.hpp"

include debug.hpp too (you use assert)

src/hotspot/os/aix/os_perf_aix.cpp line 48:

> 46: #include <sys/types.h>
> 47: #include <stdlib.h>
> 48: #include <unistd.h>

nice reordering

src/hotspot/os/aix/os_perf_aix.cpp line 94:

> 92:   int len;
> 93: 
> 94:   memset(buf, 0, BUF_LENGTH);

memset is not needed

src/hotspot/os/aix/os_perf_aix.cpp line 95:

> 93: 
> 94:   memset(buf, 0, BUF_LENGTH);
> 95:   snprintf(buf, BUF_LENGTH, "/proc/%llu/psinfo", pid);

Use jio_snprintf instead, it handles a number of special cases (e.g. truncation) more correctly. Needs, I believe, jvm_io.h.

src/hotspot/os/aix/os_perf_aix.cpp line 102:

> 100:   }
> 101: 
> 102:   len = fread(&psinfo, sizeof(char), sizeof(psinfo_t), fp);

bikeshedding: I think its safe to say that sizeof(char) will always be 1, so I guess you can shorten it to 1 :)

src/hotspot/os/aix/os_perf_aix.cpp line 121:

> 119:     pticks->sys  = 0;
> 120:     pticks->idle = 0;
> 121:     pticks->wait = 0;

bikeshed: memset to 0 instead of setting each member individually?

src/hotspot/os/aix/os_perf_aix.cpp line 146:

> 144:   assert(initialize_libperfstat(), "perfstat lib not available");
> 145: 
> 146:   snprintf(name_holder.name, IDENTIFIER_LENGTH, "%d", getpid());

jio_snprintf

src/hotspot/os/aix/os_perf_aix.cpp line 214:

> 212:   // populate cpu_stats && check that the expected number of records have been populated
> 213:   if (ncpus != libperfstat::perfstat_cpu(&name_holder, all_lcpu_stats, sizeof(perfstat_cpu_t), ncpus)) {
> 214:     FREE_RESOURCE_ARRAY(perfstat_cpu_t, all_lcpu_stats, ncpus);

ResourceArray are stack-based arenas. No need to free them manually (freeing is often a noop anyway unless the allocation is a the top of the arena). Very similar to those AutoReleasePools in Objective-C, if you know that.

The way to use them is don't release them. If those allocations don't escape the function (so, you don't return RA memory from the function), you can set a ResourceMark at the start of the function. That is an RAII object which will clear all RA objects allocated in and under this function upon return.

Arguably, you also could just use malloc and free here (NEW_C_HEAP_ARRAY and FREE_C_HEAP_ARRAY)

src/hotspot/os/aix/os_perf_aix.cpp line 516:

> 514:   }
> 515: 
> 516:   FREE_RESOURCE_ARRAY(perfstat_process_t, proc_stats, records_allocated);

no need for release. Use ResourceMark at the top of the function unless RA memory escapes. Or, C heap.

src/hotspot/os/aix/os_perf_aix.cpp line 639:

> 637:   }
> 638: 
> 639:   FREE_RESOURCE_ARRAY(perfstat_netinterface_t, net_stats, records_allocated);

same

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

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6885