RFR: 8134540: Much nearly duplicated code for PerfMemory support [v2]

[Thomas Stuefe]

On Fri, 15 Jan 2021 14:13:22 GMT, Harold Seigel <hseigel at openjdk.org> wrote:

>> Please review this change to merge the Unix-like PerfMemory_*.cpp source files into a single PerfMemory_posix.cpp source file.
>> 
>> The changes were tested on Mach5 tiers 1-5 on Linux, Windows, and Mac OS, and cross-builds were done for the S390, arm32, ppc, and x86 platforms.
>> 
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8134540: changes for review comments

Hi Harold,

thanks a lot for this! I think most of the AIX peculiarities are just bitrots (us missing changes to other platforms and forgetting to side-merge). Thankfully this becomes posix shared code now.

Thanks, Thomas

src/hotspot/os/posix/os_posix.inline.hpp line 45:

> 43: 
> 44: 
> 45: inline int os::close(int fd) {

Does this have to be in the inline header? not, simply, os_posix.cpp?

src/hotspot/os/posix/perfMemory_posix.cpp line 1026:

> 1024: #else
> 1025:   MemTracker::record_virtual_memory_reserve_and_commit((address)mapAddress, size, CURRENT_PC, mtInternal);
> 1026: #endif

I think the AIX specific path can b e removed. Its committed on AIX too.

src/hotspot/os/posix/perfMemory_posix.cpp line 1040:

> 1038:     warning("perfmemory: munmap failed (%d)\n", errno);
> 1039:   }
> 1040: #else

- I think the AIX specific path would be more correct for all other implementations too. Since on creation, we call raw ::mmap(). Then we should call raw ::munmap here too. Otherwise this relies on the fact that os::reserve_memory always uses mmap(), which is not given.
- Strictly speaking this needs a release from MemTracker if it does not go thru os::release_memory. But I think its not important, this mapping only goes away when the VM exits, no? At least on AIX no-one ever noticed anything.

src/hotspot/os/posix/perfMemory_posix.cpp line 1213:

> 1211: #if defined(_AIX)
> 1212:   MemTracker::record_virtual_memory_reserve((address)mapAddress, size, CURRENT_PC, mtInternal);
> 1213: #else

As above, the generic path should be fine for AIX too.

src/hotspot/os/posix/perfMemory_posix.cpp line 895:

> 893:   for (size_t seekpos = 0; seekpos < size; seekpos += os::vm_page_size()) {
> 894:     int zero_int = 0;
> 895:     result = (int)os::seek_to_file_offset(fd, (jlong)(seekpos));

os::seek_to_file_offset could, at least for the posix platforms, also be unified (separate RFE).

src/hotspot/os/posix/perfMemory_posix.cpp line 889:

> 887:   }
> 888: 
> 889: #if !defined(_AIX)

Please enable this for AIX too. Seems we just missed "JDK-7007769: VM crashes with SIGBUS writing PerfData if tmp space is full" (I think AIX port was not official then)

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

Changes requested by stuefe (Reviewer).

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