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

[Harold Seigel]

On Thu, 14 Jan 2021 07:35:45 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8134540: changes for review comments
>
> 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.

Changed to use 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"!

Changed to use S_IRUSR|S_IWUSR.

> 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?

It is not Linux specific.  AIX and BSD also use mmap.  So I changed the comment to say "Note: the Posix shared ..."

> 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?

This comment is incorrect for Linux because of containers. So, I deleted the comment.

> 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.

done

> 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.

done

> 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.

done

> 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.

done

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

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