On Thu, 27 Jan 2022 00:15:52 GMT, Tyler Steele <d...@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
