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

[Tyler Steele]

On Thu, 27 Jan 2022 00:15:52 GMT, Tyler Steele <duke at openjdk.java.net> wrote:

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

I performed the refactor (removing entry_t, and adding a next pointer to loaded_module_t). This was pretty straightforward, but required writing an explicit copy procedure since the new struct loaded_module_t no longer met the requirements of the compiler-generated one.

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

This is a good suggestion. I thought about doing it before, but I am used to tamping down my functional programming instincts when writing C. That said, this option required no additional memory allocation, so it removes any need for the class I wrote before. My implementation is a little brittle, as I chose to take os::LoadedModulesCallbackFunc, rather than a more general type (and having to construct a closure etc.). This should save on indirection for now, and could be extended in the future if needed.

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

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