On Fri, 11 Aug 2023 09:31:56 GMT, Yi Yang <yy...@openjdk.org> wrote:
> This patch reduce ~16%(24s->20s) pahse 2 merge time during dumping 32g heap
> with 96threads and fixes a memory leak of compressor
>
> You might argue why this is Linux-only optimization, because sendfile
> requires at least socket fd in other platforms([aix
> sendfile](https://www.ibm.com/docs/en/aix/7.1?topic=s-send-file-subroutine)
> [maxos
> sendfile](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/sendfile.2.html)
> [win32
> TransmitFile](https://learn.microsoft.com/en-us/windows/win32/api/mswsock/nf-mswsock-transmitfile)),
> while [only Linux](https://man7.org/linux/man-pages/man2/sendfile.2.html)
> supports both two file descriptors.
The fix looks good to me in general, but I'm not sure about code organization.
src/hotspot/share/runtime/os.hpp describes rules for os* files.
It states:
// Platform-independent source files should not include these header files
// (although sadly there are some rare exceptions ...)
And the change adds one more exception.
I'd like to hear runtime guys opinion.
src/hotspot/share/services/heapDumper.cpp line 1651:
> 1649: // read+write combination, which would require transferring data
> to and from
> 1650: // user space.
> 1651: merge_file_fast(path);
Looks like you don't need separate method for linux.
Would it be better to make linux-specific implementation of merge_file():
#ifdef LINUX
// Merge segmented heap files via sendfile
void DumpMerger::merge_file(char* path) {
...
}
#else
// Generic implementation using read+write
void DumpMerger::merge_file(char* path) {
...
}
#endif
-------------
PR Comment: https://git.openjdk.org/jdk/pull/15245#issuecomment-1709311297
PR Review Comment: https://git.openjdk.org/jdk/pull/15245#discussion_r1317959545
