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

> 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 incrementally with one additional 
commit since the last revision:

  Removes incorrect Oracle copyright line from libperfstat_aix.cpp/hpp, and 
loadlib_aix.hpp

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/4ab28477..0551e65e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=18
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=17-18

  Stats: 3 lines in 3 files changed: 0 ins; 3 del; 0 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

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

On Wed, 2 Feb 2022 17:17:10 GMT, Martin Doerr  wrote:

> Thanks for the update. As David had written, the Oracle Copyright lines you 
> have added are not correct: "Copyright (c) 2022, 2022". I suggest to avoid 
> adding new lines for this change until the topic is clarified.

Agreed. I have removed the incorrect header line and will add a new one when I 
receive clarification.

-

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

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

2022-02-02 Thread Martin Doerr

On Wed, 2 Feb 2022 16:30:56 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 incrementally with one additional 
> commit since the last revision:
> 
>   Edit thread_aix.cpp to match thread_linux.cpp in 
> pd_get_top_fram_for_profiling and ...for_signal_handler

Thanks for the update. As David had written, the Oracle Copyright lines you 
have added are not correct: "Copyright (c) 2022, 2022". I suggest to avoid 
adding new lines for this change until the topic is clarified.

-

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

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

On Fri, 21 Jan 2022 22:48:20 GMT, Martin Doerr  wrote:

>> Tyler Steele has refreshed the contents of this pull request, and previous 
>> commits have been removed. Incremental views are not available.
>
> src/hotspot/os_cpu/aix_ppc/thread_aix_ppc.cpp line 63:
> 
>> 61: 
>> 62: if (ret_frame.pc() == NULL) {
>> 63:   // ucontext wasn't useful
> 
> Null check has been moved before frame constructor in head revision.

Thanks for the reminder. I made this change in a recent push.

-

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

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

> 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 incrementally with one additional 
commit since the last revision:

  Edit thread_aix.cpp to match thread_linux.cpp in 
pd_get_top_fram_for_profiling and ...for_signal_handler

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/3618a77d..4ab28477

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=17
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=16-17

  Stats: 8 lines in 1 file changed: 2 ins; 2 del; 4 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

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

2022-02-02 Thread Martin Doerr

On Tue, 1 Feb 2022 21:36:56 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 incrementally with one additional 
> commit since the last revision:
> 
>   Addresses issues from review and other sm fixes
>   
>   - Adds commenting in regards to memory handling by SystemProcess &
>   NetworkInterface classes
>   - Replaces explicit initialization and copy of structs with memcpy
>   and memset as appropriate
>   - Renames internal struct definitions in os_perf_aix
>   - Other minor fixes

Please don't forget src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp update 
https://github.com/openjdk/jdk/commit/f37bfeadcf036a75defc64ad7f4a9f5596cd7407

-

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

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

2022-02-02 Thread Thomas Stuefe

On Tue, 1 Feb 2022 21:36:56 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 incrementally with one additional 
> commit since the last revision:
> 
>   Addresses issues from review and other sm fixes
>   
>   - Adds commenting in regards to memory handling by SystemProcess &
>   NetworkInterface classes
>   - Replaces explicit initialization and copy of structs with memcpy
>   and memset as appropriate
>   - Renames internal struct definitions in os_perf_aix
>   - Other minor fixes

Looks good to me now. 

Can you now use JFR/JMC on AIX with this patch or is further work needed?

Thanks for doing this!

Cheers, Thomas

-

Marked as reviewed by stuefe (Reviewer).

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

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

2022-02-01 Thread Tyler Steele

On Sat, 29 Jan 2022 06:51:59 GMT, Thomas Stuefe  wrote:

>> Tyler Steele has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Changes macoss -> macosx in problem list
>>  - Refactors loadlib_aix: Removes redundant c++ class
>
> src/hotspot/os/aix/os_perf_aix.cpp line 631:
> 
>> 629:
>> net_stats[i].ibytes,
>> 630:
>> net_stats[i].obytes,
>> 631:
>> *network_interfaces);
> 
> When looking at NetworkInterface, I saw that it copies the name into its own 
> resource-area allocated string in the constructor. This is all over the 
> place, SystemProcess assumes C-heap storage, NetworkInterface uses RA... :(
> 
> The problem now is that your ResourceMark above may also destroy the internal 
> string in NetworkInterface when the Stack is unwound beyond this function.
> 
> Side note: this is what I don't like about how hotspot uses RA. Its often not 
> clear which data are RA allocated without looking at the code. In Objective-C 
> (at least how its used on Mac) they had a clear naming convention for methods 
> that returned storage from the AutoReleasePool.
> 
> The problem is also that with ResourceMarks cutting off your data, you may 
> not see the problem until much later. Usually you only see immediate problems 
> if the underlying arena (a chained list of malloced slabs) added a new slab 
> under your ResourceMark, which then gets free'd when the ResourceMark goes 
> out of scope.
> 
> How to fix this:
> 1) revert to NEW_C_HEAP_ARRAY for your records array here, with appropriate 
> cleanup, and remove ResourceMark
> 3) Change NetworkInterface class to use C-heap instead.
> 
> I'd opt for (1) since it avoids discussions about changing shared code. I 
> opened https://bugs.openjdk.java.net/browse/JDK-8280912 to change 
> NetworkInterface, but don't wait for me.

I made the change in (1) as recommended.

-

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

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

2022-02-01 Thread Tyler Steele

> 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 incrementally with one additional 
commit since the last revision:

  Addresses issues from review and other sm fixes
  
  - Adds commenting in regards to memory handling by SystemProcess &
  NetworkInterface classes
  - Replaces explicit initialization and copy of structs with memcpy
  and memset as appropriate
  - Renames internal struct definitions in os_perf_aix
  - Other minor fixes

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/08837098..3618a77d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=16
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=15-16

  Stats: 108 lines in 2 files changed: 19 ins; 56 del; 33 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

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

On Sat, 29 Jan 2022 06:36:32 GMT, Thomas Stuefe  wrote:

>> Tyler Steele has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Changes macoss -> macosx in problem list
>>  - Refactors loadlib_aix: Removes redundant c++ class
>
> src/hotspot/os/aix/os_perf_aix.cpp line 495:
> 
>> 493: char* name = NEW_C_HEAP_ARRAY(char, IDENTIFIER_LENGTH, 
>> mtInternal);
>> 494: char* exe_name = NEW_C_HEAP_ARRAY(char, PRFNSZ, mtInternal);
>> 495: char* cmd_line = NEW_C_HEAP_ARRAY(char, PRARGSZ, mtInternal);
> 
> So the contract with SystemProcess is that its ctor arguments need to be 
> C-heap allocated, and releases them itself? ... okay. May be worthwhile to 
> clean up at some point, or at least comment.

Yeah, this was a bit of a gotcha during development (especially since 
NetworkInterface is different); I agree it's worth documenting.

-

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

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

On Sat, 29 Jan 2022 06:30:01 GMT, Thomas Stuefe  wrote:

>> Tyler Steele has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Changes macoss -> macosx in problem list
>>  - Refactors loadlib_aix: Removes redundant c++ class
>
> src/hotspot/os/aix/os_perf_aix.cpp line 72:
> 
>> 70: 
>> 71: static bool initialize_libperfstat() {
>> 72:   static bool is_libperfstat_loaded = false;
> 
> I don't think this is needed. libperfstat is initialized as part of os::init 
> in os_aix.cpp.
> 
> https://github.com/openjdk/jdk/blob/d366d15d67a08833d93a5806edef8145cb7803e5/src/hotspot/os/aix/os_aix.cpp#L2999-L3008
> https://github.com/openjdk/jdk/blob/d366d15d67a08833d93a5806edef8145cb7803e5/src/hotspot/os/aix/os_aix.cpp#L2329-L2333
> 
> Should be already active at this point. Its functions are stubbed out in case 
> the library cannot be loaded, so you should be able to call them in here, 
> without an additional init check. You just have to handle the errors on the 
> individual API calls, as if the real APIs returned errors. But you do this 
> already I think.
> 
> Long term, if you think libperfstat should be always there, we can simplify 
> things and treat libperfstat load errors as hard VM errors in release builds 
> too. We already assert in `os::Aix::initialize_libperfstat()`. (Note: 
> "assert" fires in debug builds (ASSERT macro), and "guarantee" fires in all 
> builds).

Aha! I saw the definition of initialize_libperfstat(), but not that it was 
later called in os::init(). It did always feel strange to manually initialize 
libperfstat as I was doing. Thanks for pointing this out.

> src/hotspot/os/aix/os_perf_aix.cpp line 611:
> 
>> 609: 
>> 610:   // calling perfstat_(NULL, NULL, _, 0) returns number of 
>> available records
>> 611:   if ((n_records = libperfstat::perfstat_netinterface(NULL, NULL, 
>> sizeof(perfstat_netinterface_t), 0)) <= 0) {
> 
> is n=0 an error? Could there conceivably be zero interfaces?

Good point. I agree it's not reasonable to return an error if there are simply 
no network interfaces.

-

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

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

On Sat, 29 Jan 2022 06:09:17 GMT, Thomas Stuefe  wrote:

>> Tyler Steele has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Changes macoss -> macosx in problem list
>>  - Refactors loadlib_aix: Removes redundant c++ class
>
> src/hotspot/os/aix/loadlib_aix.cpp line 326:
> 
>> 324: // Helper for copying loaded module info to external callers.
>> 325: // To avoid dangling pointers 'next' is set to null
>> 326: static bool copy_lm_to_external(const loaded_module_t* const from, 
>> loaded_module_t* to) {
> 
> So, just to understand, the point of this function is to handle to==NULL 
> (from is never NULL) and to set next to NULL? 
> 
> I'd probably keep doing this inline in the two places where this happens, but 
> I leave it up to you. If you think this is cleaner, its fine too.

The purpose was actually to work through a compiler error that cropped up when 
I changed loaded_module_t. It ended up being simple to fix, but I left the 
plumbing in. I agree this is not worth keeping around. Thanks for pointing this 
out.

-

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

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

On Sat, 29 Jan 2022 05:58:27 GMT, Thomas Stuefe  wrote:

>> Tyler Steele has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Changes macoss -> macosx in problem list
>>  - Refactors loadlib_aix: Removes redundant c++ class
>
> src/hotspot/os/aix/loadlib_aix.cpp line 116:
> 
>> 114: //   entry_t* next;
>> 115: //   loaded_module_t info;
>> 116: // };
> 
> please remove

Agreed. Thanks for catching this.

> src/hotspot/os/aix/os_perf_aix.cpp line 31:
> 
>> 29: #include "memory/resourceArea.hpp"
>> 30: #include "os_aix.inline.hpp"
>> 31: #include "precompiled.hpp"
> 
> precompiled.hpp must appear at the very top of the cpp file. The one 
> exception from the "order includes alphabetically" rule. Oh, that, and people 
> usually include system headers after hotspot ones.

👍 

Also, since I'm thinking of it here. I ran without precompiled.hpp as you 
mentioned elsewhere. No issues detected.

-

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

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

2022-01-28 Thread Thomas Stuefe

On Fri, 28 Jan 2022 15:07:56 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 incrementally with two additional 
> commits since the last revision:
> 
>  - Changes macoss -> macosx in problem list
>  - Refactors loadlib_aix: Removes redundant c++ class

Hi Tyler,

looks better. See comments inline.

src/hotspot/os/aix/loadlib_aix.cpp line 116:

> 114: //   entry_t* next;
> 115: //   loaded_module_t info;
> 116: // };

please remove

src/hotspot/os/aix/loadlib_aix.cpp line 326:

> 324: // Helper for copying loaded module info to external callers.
> 325: // To avoid dangling pointers 'next' is set to null
> 326: static bool copy_lm_to_external(const loaded_module_t* const from, 
> loaded_module_t* to) {

So, just to understand, the point of this function is to handle to==NULL (from 
is never NULL) and to set next to NULL? 

I'd probably keep doing this inline in the two places where this happens, but I 
leave it up to you. If you think this is cleaner, its fine too.

src/hotspot/os/aix/loadlib_aix.hpp line 37:

> 35: 
> 36: #include "misc_aix.hpp"
> 37: #include "runtime/os.hpp"

Bikeshed: I would like to have avoided including os.hpp (its a monster) - hence 
my recommendation about a separate callback or closure definition. But it is 
not super important, this is fine too.

src/hotspot/os/aix/os_perf_aix.cpp line 72:

> 70: 
> 71: static bool initialize_libperfstat() {
> 72:   static bool is_libperfstat_loaded = false;

I don't think this is needed. libperfstat is initialized as part of os::init in 
os_aix.cpp.

https://github.com/openjdk/jdk/blob/d366d15d67a08833d93a5806edef8145cb7803e5/src/hotspot/os/aix/os_aix.cpp#L2999-L3008
https://github.com/openjdk/jdk/blob/d366d15d67a08833d93a5806edef8145cb7803e5/src/hotspot/os/aix/os_aix.cpp#L2329-L2333

Should be already active at this point. Its functions are stubbed out in case 
the library cannot be loaded, so you should be able to call them in here, 
without an additional init check. You just have to handle the errors on the 
individual API calls, as if the real APIs returned errors. But you do this 
already I think.

Long term, if you think libperfstat should be always there, we can simplify 
things and treat libperfstat load errors as hard VM errors in release builds 
too. We already assert in `os::Aix::initialize_libperfstat()`. (Note: "assert" 
fires in debug builds (ASSERT macro), and "guarantee" fires in all builds).

src/hotspot/os/aix/os_perf_aix.cpp line 495:

> 493: char* name = NEW_C_HEAP_ARRAY(char, IDENTIFIER_LENGTH, 
> mtInternal);
> 494: char* exe_name = NEW_C_HEAP_ARRAY(char, PRFNSZ, mtInternal);
> 495: char* cmd_line = NEW_C_HEAP_ARRAY(char, PRARGSZ, mtInternal);

So the contract with SystemProcess is that its ctor arguments need to be C-heap 
allocated, and releases them itself? ... okay. May be worthwhile to clean up at 
some point, or at least comment.

src/hotspot/os/aix/os_perf_aix.cpp line 611:

> 609: 
> 610:   // calling perfstat_(NULL, NULL, _, 0) returns number of 
> available records
> 611:   if ((n_records = libperfstat::perfstat_netinterface(NULL, NULL, 
> sizeof(perfstat_netinterface_t), 0)) <= 0) {

is n=0 an error? Could there conceivably be zero interfaces?

src/hotspot/os/aix/os_perf_aix.cpp line 631:

> 629:
> n

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

2022-01-28 Thread Thomas Stuefe

On Fri, 28 Jan 2022 15:07:56 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 incrementally with two additional 
> commits since the last revision:
> 
>  - Changes macoss -> macosx in problem list
>  - Refactors loadlib_aix: Removes redundant c++ class

src/hotspot/os/aix/os_perf_aix.cpp line 31:

> 29: #include "memory/resourceArea.hpp"
> 30: #include "os_aix.inline.hpp"
> 31: #include "precompiled.hpp"

precompiled.hpp must appear at the very top of the cpp file. The one exception 
from the "order includes alphabetically" rule. Oh, that, and people usually 
include system headers after hotspot ones.

-

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

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

2022-01-28 Thread Tyler Steele

> 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 incrementally with two additional 
commits since the last revision:

 - Changes macoss -> macosx in problem list
 - Refactors loadlib_aix: Removes redundant c++ class

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/bcfb25be..08837098

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=15
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=14-15

  Stats: 163 lines in 5 files changed: 27 ins; 54 del; 82 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

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

2022-01-28 Thread Tyler Steele

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]

2022-01-28 Thread Magnus Ihse Bursie

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]

2022-01-27 Thread Thomas Stuefe

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]

2022-01-27 Thread David Holmes

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]

2022-01-27 Thread Tyler Steele

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]

2022-01-27 Thread Tyler Steele

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]

2022-01-27 Thread Tyler Steele

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]

2022-01-27 Thread Goetz Lindenmaier

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]

2022-01-27 Thread Thomas Stuefe

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]

2022-01-26 Thread David Holmes


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]

2022-01-26 Thread Tyler Steele

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]

2022-01-25 Thread Erik Joelsson

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]

2022-01-25 Thread David Holmes


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]

2022-01-25 Thread Thomas Stuefe

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) [v10]

On Mon, 24 Jan 2022 16:58:38 GMT, Martin Doerr  wrote:

> > Thanks for your comments @TheRealMDoerr. I agree that 131 files changed is 
> > too many to review; most of them are not files I actually changed, so I 
> > will do some digging to understand what happened there. My typical git 
> > workflow does not seem to be working well for this project.
> 
> I believe you can click on "edit" and then select which branch to use to 
> create the diff.

I was not able to edit the branch, but I was able to find another work around. 
This most recent commit [consolidates] the messy commit history [into a single 
commit], and only references the 10 files I actually 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

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

> 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 one commit:

  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=13
  Stats: 1195 lines in 10 files changed: 500 ins; 481 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

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

2022-01-24 Thread Martin Doerr

On Fri, 21 Jan 2022 09:28:27 GMT, Martin Doerr  wrote:

>> Tyler Steele has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Revert changes to jfrThreadSampler.cpp
>>  - Update copyright years. Remove 'TODO' where no longer needed.
>
> Thanks for enhancing libperfstat_aix and making it consistent.
> I just noticed that JavaThread::pd_get_top_frame_for_profiling does not yet 
> contain all the required checks. I think they should be taken from 
> thread_linux_ppc.cpp (note that there were recent fixes).

> Thanks for your comments @TheRealMDoerr. I agree that 131 files changed is 
> too many to review; most of them are not files I actually changed, so I will 
> do some digging to understand what happened there. My typical git workflow 
> does not seem to be working well for this project.

I believe you can click on "edit" and then select which branch to use to create 
the diff.

> Regarding the changes requested to 
> `JavaThread::pd_get_top_frame_for_profiling`, is there an issue/pr for 
> tracking the changes you are requesting?

https://github.com/openjdk/jdk/commit/f37bfeadcf036a75defc64ad7f4a9f5596cd7407

-

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

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

On Fri, 21 Jan 2022 09:28:27 GMT, Martin Doerr  wrote:

>> Tyler Steele has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Revert changes to jfrThreadSampler.cpp
>>  - Update copyright years. Remove 'TODO' where no longer needed.
>
> Thanks for enhancing libperfstat_aix and making it consistent.
> I just noticed that JavaThread::pd_get_top_frame_for_profiling does not yet 
> contain all the required checks. I think they should be taken from 
> thread_linux_ppc.cpp (note that there were recent fixes).

Thanks for your comments @TheRealMDoerr. I agree that 131 files changed is too 
many to review; most of them are not files I actually changed, so I will do 
some digging to understand what happened there. My typical git workflow does 
not seem to be working well for this project.

Regarding the changes requested to 
`JavaThread::pd_get_top_frame_for_profiling`, is there an issue/pr for tracking 
the changes you are requesting?

-

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

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

2022-01-21 Thread Martin Doerr

On Fri, 21 Jan 2022 17:35:14 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 incrementally with one additional 
> commit since the last revision:
> 
>   Copies updates from thread_linux_ppc.cpp to thread_aix.cpp

I think this PR needs to get fixed such that it compares to the rebased 
revision. 131 are not reviewable.

src/hotspot/os_cpu/aix_ppc/thread_aix_ppc.cpp line 62:

> 60:  (address)uc->uc_mcontext.regs->nip);
> 61: 
> 62: if (ret_frame.pc() == NULL) {

Null check has been moved before frame constructor in head revision.

-

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

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

> 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 incrementally with one additional 
commit since the last revision:

  Copies updates from thread_linux_ppc.cpp to thread_aix.cpp

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/49e0b9bc..999de23a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=12
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=11-12

  Stats: 61 lines in 1 file changed: 57 ins; 0 del; 4 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

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

On Wed, 19 Jan 2022 16:07:36 GMT, Tyler Steele  wrote:

>> make/autoconf/flags-cflags.m4 line 421:
>> 
>>> 419: DEBUG_SYMBOLS_CFLAGS_JDK="$CFLAGS_DEBUG_SYMBOLS"
>>> 420:   fi
>>> 421: 
>> 
>> Why this removal? Note that getting the TOC not to explode on AIX has been 
>> an ongoing struggle, see this string of associated JBS issues: 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8184344
>> https://bugs.openjdk.java.net/browse/JDK-8171408
>> https://bugs.openjdk.java.net/browse/JDK-8196488
>> https://bugs.openjdk.java.net/browse/JDK-8204935
>
> Hmm, that is strange. I initially thought my code had caused the TOC 
> overflow, and added  -qpic=large to address this. After realizing that my 
> code was not solely responsible, I (tried) to back out of my changes without 
> removing anything I had not added myself. It seems I should review the 
> changes to this file one more time to ensure that I didn't get a bit carried 
> away with the removal process. Good catch.

I checked out the version of this file from upstream/master. All the changes I 
needed to make were in jvm-features.m4.

-

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

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

> 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 incrementally with one additional 
commit since the last revision:

  Re-removes changes to flags-cflags.m4

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/50cf00e1..49e0b9bc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=11
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=10-11

  Stats: 8 lines in 1 file changed: 0 ins; 8 del; 0 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

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

> 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 42 commits:

 - rebase on upstream/master to pick up project changes
 - Revert changes to jfrThreadSampler.cpp
 - Update copyright years. Remove 'TODO' where no longer needed.
 - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
 - Merge branch 'master' into JDK-8203290
 - Merge branch 'JDK-8203290' into JDK-8203290-testing
 - Clean up & testing
   
   - Run JFR tests in test/jdk/jdk/jfr
   - Fix issues found from above test suite
   - Remove unecessary jtreg & gtest tests
   - Remove ineffective `qpic=large`
   - TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
 - Merge branch 'JDK-8203290-testing' of github.com:backwaterred/jdk into 
JDK-8203290-testing
 - Reverts flags-cflags.m4
 - wip
 - ... and 32 more: https://git.openjdk.java.net/jdk/compare/35172cda...50cf00e1

-

Changes: https://git.openjdk.java.net/jdk/pull/6885/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=10
  Stats: 1134 lines in 10 files changed: 446 ins; 479 del; 209 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

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

2022-01-21 Thread Martin Doerr

On Thu, 20 Jan 2022 22:51:28 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 incrementally with two additional 
> commits since the last revision:
> 
>  - Revert changes to jfrThreadSampler.cpp
>  - Update copyright years. Remove 'TODO' where no longer needed.

Thanks for enhancing libperfstat_aix and making it consistent.
I just noticed that JavaThread::pd_get_top_frame_for_profiling does not yet 
contain all the required checks. I think they should be taken from 
thread_linux_ppc.cpp (note that there were recent fixes).

-

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

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

2022-01-20 Thread Tyler Steele

> 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 incrementally with two additional 
commits since the last revision:

 - Revert changes to jfrThreadSampler.cpp
 - Update copyright years. Remove 'TODO' where no longer needed.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/e18d56bc..5cbf393a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=08-09

  Stats: 11 lines in 8 files changed: 3 ins; 4 del; 4 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

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

2022-01-20 Thread Tyler Steele

> 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 incrementally with 19 additional 
commits since the last revision:

 - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
 - Merge branch 'JDK-8203290' into JDK-8203290-testing
 - Merge branch 'JDK-8203290-testing' of github.com:backwaterred/jdk into 
JDK-8203290-testing
 - Reverts flags-cflags.m4
 - wip
 - wip
 - Housekeeping
   
   - Minor cleanups
   - Fetch additional SystemProcess info from /proc//psinfo file
   - Implement Event creation for loaded modules (fixes
   TestNativeLibrariesEvent failure)
   - Confirm working ThreadCrashPro on AIX
   - Add missing functionality to libperfstat, refactor os_perf_aix
   to use this project library.
   - [TODO] review changes to flags-cflags.m4. Only keep changes required
   for JFR build.
 - Housekeeping
   
   - Fetch additional SystemProcess info from /proc//psinfo file
   - Implement Event creation for loaded modules (fixes
 TestNativeLibrariesEvent failure)
   - Confirm working ThreadCrashPro on AIX
   - Add missing functionality to libperfstat, refactor os_perf_aix
 to use this project library.
   - Revert uneeded changes to flags-cflags.m4.
 - Merge branch 'JDK-8203290' into JDK-8203290-testing
 - Fix issue where network_interface pointer is not updated to newly created 
list
 - ... and 9 more: https://git.openjdk.java.net/jdk/compare/ceabb571...e18d56bc

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/ceabb571..e18d56bc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=07-08

  Stats: 410 lines in 8 files changed: 339 ins; 16 del; 55 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

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

2022-01-19 Thread Tyler Steele

On Tue, 18 Jan 2022 15:23:42 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 five commits:
>> 
>>  - Merge branch 'master' into JDK-8203290
>>  - Clean up & testing
>>
>>- Run JFR tests in test/jdk/jdk/jfr
>>- Fix issues found from above test suite
>>- Remove unecessary jtreg & gtest tests
>>- Remove ineffective `qpic=large`
>>- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
>>  - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
>>  - 8203290: Implements Java Flight Recorder on AIX
>>
>> - changes build system to allow jfr feature on aix
>> - implements NetworkPerformance, CPUPerformance, and SystemProcess
>> interfaces from os_perf.hpp
>> - implements jfr sanity tests
>>  - 8203290: Implements Java Flight Recorder on AIX
>>
>> - changes build system to allow jfr feature on aix
>> - implements NetworkPerformance, CPUPerformance, and SystemProcess
>> interfaces from os_perf.hpp
>> - implements jfr sanity tests
>
> make/autoconf/flags-cflags.m4 line 421:
> 
>> 419:   # so for debug we build with '-qpic=large -bbigtoc'.
>> 420:   DEBUG_CFLAGS_JVM="-qpic=large"
>> 421: fi
> 
> Why this removal? Note that getting the TOC not to explode on AIX has been an 
> ongoing struggle, see this string of associated JBS issues: 
> 
> https://bugs.openjdk.java.net/browse/JDK-8184344
> https://bugs.openjdk.java.net/browse/JDK-8171408
> https://bugs.openjdk.java.net/browse/JDK-8196488
> https://bugs.openjdk.java.net/browse/JDK-8204935

Hmm, that is strange. I initially thought my code had caused the TOC overflow, 
and added  -qpic=large to address this. After realizing that my code was not 
solely responsible, I (tried) to back out of my changes without removing 
anything I had not added myself. It seems I should review the changes to this 
file one more time to ensure that I didn't get a bit carried away with the 
removal process. Good catch.

-

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

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

2022-01-18 Thread Thomas Stuefe

On Wed, 12 Jan 2022 15:18:46 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 five commits:
> 
>  - Merge branch 'master' into JDK-8203290
>  - Clean up & testing
>
>- Run JFR tests in test/jdk/jdk/jfr
>- Fix issues found from above test suite
>- Remove unecessary jtreg & gtest tests
>- Remove ineffective `qpic=large`
>- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
>  - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests

Hi Tyler,

nice to see this effort. I'm curious, are you associated with IBM?

This is not a full review, but I can take a closer look over the next days if 
necessary (but looks like you already got your full set of reviewers).

About ThreadCrashProtection, we unified that into os_posix.cpp, so it should 
work out of the box on any Posix system. Its the usual sigsetjmp/siglongjmp 
game, covering SIGBUS and SIGSEGV. I am not aware of any AIX peculiarities 
which would prevent this from working on AIX.

Cheers, Thomas

make/autoconf/flags-cflags.m4 line 421:

> 419:   # so for debug we build with '-qpic=large -bbigtoc'.
> 420:   DEBUG_CFLAGS_JVM="-qpic=large"
> 421: fi

Why this removal? Note that getting the TOC not to explode on AIX has been an 
ongoing struggle, see this string of associated JBS issues: 

https://bugs.openjdk.java.net/browse/JDK-8184344
https://bugs.openjdk.java.net/browse/JDK-8171408
https://bugs.openjdk.java.net/browse/JDK-8196488
https://bugs.openjdk.java.net/browse/JDK-8204935

-

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

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

2022-01-17 Thread Markus Grönlund

On Wed, 12 Jan 2022 15:18:46 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 five commits:
> 
>  - Merge branch 'master' into JDK-8203290
>  - Clean up & testing
>
>- Run JFR tests in test/jdk/jdk/jfr
>- Fix issues found from above test suite
>- Remove unecessary jtreg & gtest tests
>- Remove ineffective `qpic=large`
>- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
>  - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests

You could test it like this:

In JfrThreadSampler.cpp, you have the OSThreadSampler::protected_task() 
function:

Insert the following to crash the system:

void OSThreadSampler::protected_task(const os::SuspendedThreadTaskContext& 
context) {
  +int* null_ptr = nullptr;
  +*null_ptr = 5;
...

If crash protection is enabled and active, for release builds, you will not get 
a system crash. Instead, you will get this output (in debug builds you will get 
a crash as expected):

[10.740s][error][jfr] Thread method sampler crashed
[10.786s][error][jfr] Thread method sampler crashed
[10.798s][error][jfr] Thread method sampler crashed
[10.810s][error][jfr] Thread method sampler crashed
[10.822s][error][jfr] Thread method sampler crashed
[10.834s][error][jfr] Thread method sampler crashed
[10.847s][error][jfr] Thread method sampler crashed
[10.858s][error][jfr] Thread method sampler crashed
...

-

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

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

2022-01-17 Thread Tyler Steele

On Wed, 12 Jan 2022 19:29:48 GMT, Markus Grönlund  wrote:

> Speaking of the thread sampler, to ensure stability, you also need a working 
> implementation of os::ThreadCrashProtection. I can see there is the one in 
> os_posix.cpp, and maybe that is the one already in use? Or does there need to 
> be a specialization in os_aix.cpp? Please see 
> https://bugs.openjdk.java.net/browse/JDK-8279077 for more details about crash 
> protection.

Thanks for bringing this to my attention @mgronlun. I spent some time looking 
over crash protection, and I believe the implementation in os_posix is brought 
in for >= jdk16. If I understand the project history (especially relating to 
the issue you linked), this would not necessarily be true for the project 
before jdk 16, so it will need to be considered if this change is backported.

Is there a way to be sure that ThreadCrashProtection is present on AIX? I have 
forced a segfault, and it appears to behave the same on Linux x86 as it does on 
AIX Power.

-

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

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

2022-01-17 Thread Tyler Steele

On Wed, 22 Dec 2021 04:16:49 GMT, David Holmes  wrote:

>> I don't have access to AIX, but there are 500+ tests in test/jdk/jdk/jfr you 
>> might want to try out.
>
> @egahlin  beat me to it - yes the JFR testing is primarily under jdk not 
> hotspot, so we don't really need the new sanity tests.
> 
> Also are you aware that we have `src/hotspot/os/aix/libperfstat_aix.*` ?

@dholmes-ora, That is a fair point. I implemented the functionality before I 
knew about the project library, but now that I know about it, the cleaner 
option would be to refactor my solution. I am expecting to make some changes to 
the code this week, so I will incorporate this one as well. Thanks for the 
suggestion.

-

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

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

2022-01-16 Thread David Holmes


On 15/01/2022 2:13 am, Tyler Steele wrote:

To follow up on an item mentioned above so that it is documented here: I 
reviewed the functionality from `src/hotspot/os/aix/libperfstat_aix` and found 
it didn't fit well with the needs of this pr. If it were extended in the future 
to include the ability to call at least `libperfstat_process` and 
`libperfstat_cpu` (in addition to `libperfstat_cpu_total`), then using it here 
would be a reasonable choice. There may be some other functionality required, 
but those stand out to me as the big ones.


Wouldn't adding the needed functionality to the existing libperfstat_aix 
be preferable here?


David


-

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

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

2022-01-14 Thread Tyler Steele

On Wed, 12 Jan 2022 15:18:46 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 five commits:
> 
>  - Merge branch 'master' into JDK-8203290
>  - Clean up & testing
>
>- Run JFR tests in test/jdk/jdk/jfr
>- Fix issues found from above test suite
>- Remove unecessary jtreg & gtest tests
>- Remove ineffective `qpic=large`
>- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
>  - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests

Thanks! That clarifies some things. The output for that test is currently 
empty, so it seems that the Native Libs are not being included in the recording.

-

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

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

2022-01-14 Thread Erik Gahlin

On Fri, 14 Jan 2022 16:18:37 GMT, Tyler Steele  wrote:

> > I'm looking for some clarification on the purpose 
> > TestNativeLibrariesEvent.java. Currently it fails with "Missing 
> > libraries:libjvm.so, libjava.so, libzip.so: expected true, was false". I 
> > find those libs in my build directory, and everything else seems to work. 
> > Can anyone provide an idea why this test is failing (or what the failure 
> > means)?
> 
> The only item keeping me from `/integrating` is the failing test referenced 
> above. Can anyone provide some clarification? [Edit: The test is checking a 
> (JFR) recording for the libs, not the path as I misunderstood earlier].
> 
> Another option is to simply add the test to a ProblemList.txt, but I'd prefer 
> not to unless it's really warranted.

What is the output if you run the test on AIX? 

It should print all NativeLibrary events. If you don't get any, you may want to 
look at how data is gathered for EventNativeLibrary.  If you get events, you 
might want to add one library as a sanity test.

-

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

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

2022-01-14 Thread Tyler Steele

On Tue, 11 Jan 2022 16:56:15 GMT, Tyler Steele  wrote:

> I'm looking for some clarification on the purpose 
> TestNativeLibrariesEvent.java. Currently it fails with "Missing 
> libraries:libjvm.so, libjava.so, libzip.so: expected true, was false". I find 
> those libs in my build directory, and everything else seems to work. Can 
> anyone provide an idea why this test is failing (or what the failure means)?

The only item keeping me from `/integrating` is the failing test referenced 
above. Can anyone provide some clarification? I find the 'missing' libs in a 
few places in the build directory including the paths below:

build/aix-ppc64-server-release/images/jdk/lib/libjava.so
build/aix-ppc64-server-release/images/jdk/lib/libzip.so
build/aix-ppc64-server-release/images/jdk/lib/server/libjvm.so

build/aix-ppc64-server-release/jdk/lib/libjava.so
build/aix-ppc64-server-release/jdk/lib/libzip.so
build/aix-ppc64-server-release/jdk/lib/server/libjvm.so

Another option is to simply add the test to a ProblemList.txt, but I'd prefer 
not to unless it's really warranted.

-

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

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

2022-01-14 Thread Tyler Steele

On Wed, 12 Jan 2022 15:18:46 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 five commits:
> 
>  - Merge branch 'master' into JDK-8203290
>  - Clean up & testing
>
>- Run JFR tests in test/jdk/jdk/jfr
>- Fix issues found from above test suite
>- Remove unecessary jtreg & gtest tests
>- Remove ineffective `qpic=large`
>- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
>  - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests

To follow up on an item mentioned above so that it is documented here: I 
reviewed the functionality from `src/hotspot/os/aix/libperfstat_aix` and found 
it didn't fit well with the needs of this pr. If it were extended in the future 
to include the ability to call at least `libperfstat_process` and 
`libperfstat_cpu` (in addition to `libperfstat_cpu_total`), then using it here 
would be a reasonable choice. There may be some other functionality required, 
but those stand out to me as the big ones.

-

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

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

2022-01-12 Thread Markus Grönlund

On Wed, 12 Jan 2022 15:18:46 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 five commits:
> 
>  - Merge branch 'master' into JDK-8203290
>  - Clean up & testing
>
>- Run JFR tests in test/jdk/jdk/jfr
>- Fix issues found from above test suite
>- Remove unecessary jtreg & gtest tests
>- Remove ineffective `qpic=large`
>- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
>  - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests
>  - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests

Hi Tyler,

Good work in getting JFR ported - thank you. Most of the work revolved around 
implementing the platform-specific os_perf* functionality. I had assumed you 
would need more work concerning the thread sampler, i.e. JFR method sampling, 
but it seems that os/posix/signals_posix.cpp can be used as-is (for suspend / 
resume) and that  pd_get_top_frame_for_profiling() already have implementations 
- that's good in that case.
Speaking of the thread sampler, to ensure stability, you also need a working 
implementation of os::ThreadCrashProtection. I can see there is the one in 
os_posix.cpp, and maybe that is the one already in use? Or does there need to 
be a specialization in os_aix.cpp? Please see 
https://bugs.openjdk.java.net/browse/JDK-8279077 for more details about crash 
protection.

I can't comment on the os_perf_aix.cpp changes from a ppc/aix viewpoint - I 
think Martin has taken care of that already.

I am ok with this from a JFR standpoint.

Thanks again
Markus

-

Marked as reviewed by mgronlun (Reviewer).

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

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

2022-01-12 Thread Tyler Steele

> 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 five commits:

 - Merge branch 'master' into JDK-8203290
 - Clean up & testing
   
   - Run JFR tests in test/jdk/jdk/jfr
   - Fix issues found from above test suite
   - Remove unecessary jtreg & gtest tests
   - Remove ineffective `qpic=large`
   - TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
 - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
 - 8203290: Implements Java Flight Recorder on AIX
   
- changes build system to allow jfr feature on aix
- implements NetworkPerformance, CPUPerformance, and SystemProcess
interfaces from os_perf.hpp
- implements jfr sanity tests
 - 8203290: Implements Java Flight Recorder on AIX
   
- changes build system to allow jfr feature on aix
- implements NetworkPerformance, CPUPerformance, and SystemProcess
interfaces from os_perf.hpp
- implements jfr sanity tests

-

Changes: https://git.openjdk.java.net/jdk/pull/6885/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=07
  Stats: 831 lines in 5 files changed: 151 ins; 514 del; 166 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

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

2022-01-12 Thread Martin Doerr

On Tue, 11 Jan 2022 16:53:05 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 refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   Clean up & testing
>   
>   - Run JFR tests in test/jdk/jdk/jfr
>   - Fix issues found from above test suite
>   - Remove unecessary jtreg & gtest tests
>   - Remove ineffective `qpic=large`
>   - TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java

The change looks good to me. I only had a quick look at the new os_perf_aix 
implementation, so someone else should serve as 2nd reviewer (no Reviewer 
status required). The old code was a copy from linux as a placeholder. Note 
that we don't build and test on AIX anymore.

-

Marked as reviewed by mdoerr (Reviewer).

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

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

On Tue, 11 Jan 2022 17:10:13 GMT, Erik Joelsson  wrote:

> I can't really comment on any AIX specific compiler flags as I'm not familiar 
> with AIX and not representing a company that supports it. All I can say is 
> that the build changes look sane from a structural point of view. I don't 
> know if anyone here is able to judge the choice of AIX specific flags better 
> than yourself.

Good to know. Thanks for clarifying.

-

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

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

On Tue, 11 Jan 2022 16:53:05 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 refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   Clean up & testing
>   
>   - Run JFR tests in test/jdk/jdk/jfr
>   - Fix issues found from above test suite
>   - Remove unecessary jtreg & gtest tests
>   - Remove ineffective `qpic=large`
>   - TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java

After running the test suite mentioned above, I found and fixed several issues 
with the PR. Only one failing test remains, and I am seeking some help with it.

I'm looking for some clarification on the purpose 
TestNativeLibrariesEvent.java. Currently it fails with "Missing 
libraries:libjvm.so, libjava.so, libzip.so: expected true, was false". I find 
those libs in my build directory, and everything else seems to work. Can anyone 
provide an idea why this test is failing (or what the failure means)?

-

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

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

2022-01-11 Thread Erik Joelsson

On Mon, 3 Jan 2022 13:57:59 GMT, Erik Joelsson  wrote:

>> Tyler Steele has refreshed the contents of this pull request, and previous 
>> commits have been removed. Incremental views are not available.
>
> Build changes look ok. You will also need ok from someone in the component 
> team.

> @erikj79 Regarding the TOC warning I mentioned above:
> 
> * I now believe the TOC warning has been present while building the master 
> branch (so this PR probably isn't the root cause).
> * I've also noticed that the addition of `-qpic=large` in 
> [flags-cflags.m4](https://github.com/backwaterred/jdk/blob/79bfca43a729675da9c95a59fead373f76e9feac/make/autoconf/flags-cflags.m4#L429)
>  hasn't removed the build warning about a TOC overflow.
> 
> For the above reasons, I've removed `-qpic=large` from the line linked above.

I can't really comment on any AIX specific compiler flags as I'm not familiar 
with AIX and not representing a company that supports it. All I can say is that 
the build changes look sane from a structural point of view. I don't know if 
anyone here is able to judge the choice of AIX specific flags better than 
yourself.

-

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

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

> 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 refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  Clean up & testing
  
  - Run JFR tests in test/jdk/jdk/jfr
  - Fix issues found from above test suite
  - Remove unecessary jtreg & gtest tests
  - Remove ineffective `qpic=large`
  - TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/79bfca43..cc04740a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=05-06

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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

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

On Mon, 3 Jan 2022 13:57:59 GMT, Erik Joelsson  wrote:

>> Tyler Steele has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix issue where network_interface pointer is not updated to newly created 
>> list
>
> Build changes look ok. You will also need ok from someone in the component 
> team.

@erikj79 
Regarding the TOC warning I mentioned above:

- I now believe the TOC warning has been present while building the master 
branch (so this PR probably isn't the root cause).
- I've also noticed that the addition of `-qpic=large` in 
[flags-cflags.m4](https://github.com/backwaterred/jdk/blob/79bfca43a729675da9c95a59fead373f76e9feac/make/autoconf/flags-cflags.m4#L429)
 hasn't removed the build warning about a TOC overflow.

Because of the above, I've removed `-qpic=large` from the line above.

-

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

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

2022-01-10 Thread Tyler Steele

> 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 incrementally with six additional 
commits since the last revision:

 - Merge branch 'JDK-8203290' into JDK-8203290-testing
 - Review changes, fix failing tests
 - Remove issue preventing TestNativeLibrariesEvent.java from building
   
   - Test should now fail
 - Fix issue where network_interface pointer is not updated to newly created 
list
 - Addresses possible memory leak in network_utilization Class
 - Add misssing lib to jfr gtest

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/a286c9a4..79bfca43

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=04-05

  Stats: 337 lines in 4 files changed: 78 ins; 160 del; 99 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

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

2022-01-03 Thread Erik Joelsson

On Wed, 29 Dec 2021 23:14:52 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 incrementally with one additional 
> commit since the last revision:
> 
>   Fix issue where network_interface pointer is not updated to newly created 
> list

Build changes look ok. You will also need ok from someone in the component team.

-

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

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

> 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 incrementally with one additional 
commit since the last revision:

  Fix issue where network_interface pointer is not updated to newly created list

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/4c0b1877..a286c9a4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=03-04

  Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 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

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

> 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 incrementally with one additional 
commit since the last revision:

  Addresses possible memory leak in network_utilization Class

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/7965898a..4c0b1877

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=02-03

  Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 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

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

> 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 incrementally with one additional 
commit since the last revision:

  Add misssing lib to jfr gtest

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6885/files
  - new: https://git.openjdk.java.net/jdk/pull/6885/files/1fe36f65..7965898a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6885&range=01-02

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 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

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