RFR: 8134540: Much nearly duplicated code for PerfMemory support

[David Holmes]

On Mon, 11 Jan 2021 20:32:20 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

Hi Harold,

I had an initial look. A few comments below.

The amount of Linux specific code is a bit concerning for a shared implementation.

Thanks,
David

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

> 354: #else
> 355:   if (!is_same_fsobject(fd, dirfd(dirp))) {
> 356: #endif

I think this would be cleaner with AIX_ONLY/NOT_AIX macros.

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

> 99:   int result;
> 100: 
> 101:   RESTARTABLE(os::open(destfile, O_CREAT|O_WRONLY|O_TRUNC, S_IREAD|S_IWRITE),

Using S_IREAD/S_IWRITE seems a bit odd when libc docs describe them as "obsolete synonym provided for BSD compatibility"!

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

> 137: // Shared Memory Implementation Details
> 138: 
> 139: // Note: the linux shared memory implementation uses the mmap

Is this really specific to Linux?

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

> 144: // simple file apis.
> 145: //
> 146: // The linux implementation stores the backing store file in

Do we need this in a shared file? What do other platforms do?

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

> 152: // return the user specific temporary directory name.
> 153: //
> 154: // On linux, if containerized process, get dirname of

Can you move this comment down into the ifdef where the actual code is, please.

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

> 390: #else
> 391:   int fd = dirfd(dirp);
> 392: #endif

I think this would be cleaner with AIX_ONLY/NOT_AIX macros.

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

> 525: // the caller is expected to free the allocated memory.
> 526: //
> 527: // On Linux, if nspid != -1, look in /proc/{vmid}/root/tmp for directories

Please move this comment block into the ifdef where the code is.

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

> 727: #else
> 728:   int pid = vmid;
> 729: #endif

This would be cleaner with LINUX_ONLY/NOT_L:INUX macros.

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

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