Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Fri, 28 Jan 2022 10:40:51 GMT, Magnus Ihse Bursie 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 > > test/jdk/ProblemList.txt line 823: > >> 821: # jdk_jfr >> 822: >> 823: jdk/jfr/event/runtime/TestNetworkUtilizationEvent.java 8228990 >> macoss-all,linux-all,windows-all > > That's not how you spell `macosx`. :) Indeed it is not :-) - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Mon, 24 Jan 2022 22:23:33 GMT, Tyler Steele 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//psinfo, and /proc//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 Build changes look good. (I have no opinion on the rest of the code). `JVM_FEATURES_PLATFORM_FILTER` is not used anymore after this patch, however I think it's good to keep it if it might be needed in the future. test/jdk/ProblemList.txt line 823: > 821: # jdk_jfr > 822: > 823: jdk/jfr/event/runtime/TestNetworkUtilizationEvent.java 8228990 > macoss-all,linux-all,windows-all That's not how you spell `macosx`. :) - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Thu, 27 Jan 2022 19:28:01 GMT, Tyler Steele wrote: > > 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. You are right, it's generally good to do things the way they are done. When in Rome. But in this case, Rome already uses closures. Not textbook-like with operator overloading, but still. Just grep the hotspot sources for "Closure" and you see what I mean. > 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. I'm sure whatever you come up will be fine from your description. Note that long term you'll be the maintainer, so you will have first say in taste matters like these. - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On 27/01/2022 9:01 pm, Goetz Lindenmaier wrote: On Mon, 24 Jan 2022 22:23:33 GMT, Tyler Steele 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//psinfo, and /proc//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, IANALE :) basic rule I epouse is to never touch anyone else's copyright line On first sight, this makes sense. But it is not the reality. All external contributors are asked to update the copyright year in the Oracle Copyright line if they contribute a fix. Yes, I should expand that statement with "unless requested to by a representative of that copyright holder". :) Also, when we did the port, we added the Oracle line to many new files because they were derived from files with Oracle copyright. That makes sense too. Cheers, David - But the files in question here are special, they are some of the few that were completely developed by us, i.e., by Thomas. I think Tyler should not update the SAP Copyright, because we do not have any rights on the code he implements. I agree with Tyler that as he signed the OCA Oracle has the Copyright on the code once he opens a PR with the patch. So adding an Oracle line can not be wrong. Alternatively, he also could add his own Copyright line. Isn't there any documentation about this? All I found is this: http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html I'll ping Iris and Jesper, maybe they can shed light on this. - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Thu, 27 Jan 2022 00:15:52 GMT, Tyler Steele 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
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Tue, 25 Jan 2022 09:06:49 GMT, Thomas Stuefe 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/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)
This is interesting, and definitely good to know. I have removed all
FREE_RESOURCE_ARRAY calls and added a ResourceMark to the top of all procedures
which use RESOURCE_ARRAYs.
-
PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Mon, 24 Jan 2022 22:23:33 GMT, Tyler Steele 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//psinfo, and /proc//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 Thanks Goetz, David, and Thomas for your comments on copyrights, and thanks to Goetz for reaching out to Iris. I'll keep an eye on the mailing list for their response and make changes accordingly. - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Mon, 24 Jan 2022 22:23:33 GMT, Tyler Steele 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//psinfo, and /proc//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, IANALE :) > basic rule I epouse is to never touch anyone else's copyright line On first sight, this makes sense. But it is not the reality. All external contributors are asked to update the copyright year in the Oracle Copyright line if they contribute a fix. Also, when we did the port, we added the Oracle line to many new files because they were derived from files with Oracle copyright. But the files in question here are special, they are some of the few that were completely developed by us, i.e., by Thomas. I think Tyler should not update the SAP Copyright, because we do not have any rights on the code he implements. I agree with Tyler that as he signed the OCA Oracle has the Copyright on the code once he opens a PR with the patch. So adding an Oracle line can not be wrong. Alternatively, he also could add his own Copyright line. Isn't there any documentation about this? All I found is this: http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html I'll ping Iris and Jesper, maybe they can shed light on this. - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Thu, 27 Jan 2022 00:17:17 GMT, Tyler Steele wrote: >> 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. No, silently failing asserts shouldn't happen. That's weird. You should either get a compile time error or all should work. If it works, debug.hpp gets pulled via some other include somewhere, or because you use precompiled headers. I only did ask you to explicitly include it since it's our policy: you always include all headers which you need in your compilation unit, directly, without relying on other includes pulling them. People often forget though, and as you saw, forgetting is usually benign. There are also exceptions (e.g. system headers usually are pulled via globalDefinitions.hpp). Wish we had this written down somewhere :-/ The reason we do this is that it makes the build more robust. If everyone includes what he needs, chances that some innocuous change in an include breaks the build for faraway code. Note that it is a good practice to build - at least once, before pushing - with precompiled headers off. `--disable-precompiled-headers` configure option. I set this always, but I build on Linux, where builds are fast. Maybe AIX is too slow to always enable it. Cheers, Thomas - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On 27/01/2022 10:19 am, Tyler Steele wrote: On Tue, 25 Jan 2022 06:10:19 GMT, Thomas Stuefe 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. IANAL but a basic rule I epouse is to never touch anyone else's copyright line. If a file with a non-Oracle copyright is updated significantly by anyone they can add their own copyright line appropriate to their situation. But AFAIK only Oracle employees should add Oracle copyright lines. Cheers, David -
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Tue, 25 Jan 2022 06:10:19 GMT, Thomas Stuefe 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 >> 47: #include >> 48: #include > > 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/hotspo
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Mon, 24 Jan 2022 22:23:33 GMT, Tyler Steele 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//psinfo, and /proc//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 Build changes look good. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On 25/01/2022 7:14 pm, Thomas Stuefe wrote: 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? Just FYI that is an invalid copyright line for Oracle. There are only two years when they are different. This will cause a build failure in the validate-headers target. But I suspect this copyright line should not have been added. David -
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Mon, 24 Jan 2022 22:23:33 GMT, Tyler Steele 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//psinfo, and /proc//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 w
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
On Mon, 24 Jan 2022 22:23:33 GMT, Tyler Steele 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//psinfo, and /proc//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 I am aiming to complete work on this feature by the end of this week if possible. I have a few tier1 test failures to review before I am happy with the PR. All test/jdk/jdk/jfr test are passing, so it's possible that some of the failures are unrelated. Are there any files that have not yet been mentioned which will need to be changed? - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v15]
> 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//psinfo, and /proc//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 - Changes: https://git.openjdk.java.net/jdk/pull/6885/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=14 Stats: 1186 lines in 10 files changed: 495 ins; 477 del; 214 mod Patch: https://git.openjdk.java.net/jdk/pull/6885.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6885/head:pull/6885 PR: https://git.openjdk.java.net/jdk/pull/6885
