On Tue, 25 Jan 2022 06:10:19 GMT, Thomas Stuefe <stu...@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
