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

[Tyler Steele]

On Tue, 25 Jan 2022 06:10:19 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> 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
>
> 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?

Yes there is, but it might not be a correct one as David's and your comments seem to indicate 😏 

The thought was that as a member of a company that has signed the OCA, work I do falls under Oracle's copyright. So any files I modify ought to have their copyright years updated. When there wasn't an Oracle copyright line, I added one (this may have been the problematic choice). Any guidance on how to do this properly is appreciated.

> 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.

Thank you for the in-depth review. I appreciate the feedback. I am working on this re-factor.

> 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)

Thanks! I was wondering why my asserts were silently failing. I thought I might need to enable asserts somewhere.

> 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

Agreed. This will be removed.

> 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.

Thanks for clarifying. I've made this change.

> 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 :)

Fair point. I initially felt that this would be more clear, but I don't feel that way reading it now. I've made this change too.

> 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?

Agreed!

> 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

Done.

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

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