On Wed, 19 Jul 2023 12:42:27 GMT, Yi Yang <yy...@openjdk.org> wrote: >> ### Motivation and proposal >> Hi, heap dump brings about pauses for application's execution(STW), this is >> a well-known pain. JDK-8252842 have added parallel support to heapdump in an >> attempt to alleviate this issue. However, all concurrent threads >> competitively write heap data to the same file, and more memory is required >> to maintain the concurrent buffer queue. In experiments, we did not feel a >> significant performance improvement from that. >> >> The minor-pause solution, which is presented in this PR, is a two-phase >> segmented heap dump: >> >> - Phase 1(STW): Concurrent threads directly write data to multiple heap >> files. >> - Phase 2(Non-STW): Merge multiple heap files into one complete heap dump >> file. This process can happen outside safepoint. >> >> Now concurrent worker threads are not required to maintain a buffer queue, >> which would result in more memory overhead, nor do they need to compete for >> locks. The changes in the overall design are as follows: >> >>  >> <p align="center">Fig1. Before</p> >> >>  >> <p align="center">Fig2. After this patch</p> >> >> ### Performance evaluation >> | memory | numOfThread | CompressionMode | STW | Total | >> | -------| ----------- | --------------- | --- | ---- | >> | 8g | 1 T | N | 15.612 | 15.612 | >> | 8g | 32 T | N | 2.561725 | 14.498 | >> | 8g | 32 T | C1 | 2.3084878 | 14.198 | >> | 8g | 32 T | C2 | 10.9355128 | 21.882 | >> | 8g | 96 T | N | 2.6790452 | 14.012 | >> | 8g | 96 T | C1 | 2.3044796 | 3.589 | >> | 8g | 96 T | C2 | 9.7585151 | 20.219 | >> | 16g | 1 T | N | 26.278 | 26.278 | >> | 16g | 32 T | N | 5.231374 | 26.417 | >> | 16g | 32 T | C1 | 5.6946983 | 6.538 | >> | 16g | 32 T | C2 | 21.8211105 | 41.133 | >> | 16g | 96 T | N | 6.2445556 | 27.141 | >> | 16g | 96 T | C1 | 4.6007096 | 6.259 | >> | 16g | 96 T | C2 | 19.2965783 | 39.007 | >> | 32g | 1 T | N | 48.149 | 48.149 | >> | 32g | 32 T | N | 10.7734677 | 61.643 | >> | 32g | 32 T | C1 | 10.1642097 | 10.903 | >> | 32g | 32 T | C2 | 43.8407607 | 88.152 | >> | 32g | 96 T | N | 13.1522042 | 61.432 | >> | 32g | 96 T | C1 | 9.0954641 | 9.885 | >> | 32g | 96 T | C2 | 38.9900931 | 80.574 | >> | 64g | 1 T | N | 100.583 | 100.583 | >> | 64g | 32 T | N | 20.9233744 | 134.701 | >> | 64g | 32 T | C1 | 18.5023784 | 19.358 | >> | 64g | 32 T | C2 | 86.4748377 | 172.707 | >> | 64g | 96 T | N | 26.7374116 | 126.08 | >> | 64g | ... > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > test failure on Windows
Hi Kevin, 1. CSR > Actually are we changing the default behaviour: from "parallel if you can", > to "serial unless you specify". We are not changing the default behavior. Now the default is still parallel (unless using SerialGC or parallel can not work), the VM will automatically calculate the appropriate parallelism unless the user specifies it. Currently, this patch only adds the -parallel option to jcmd, AFAIK, this does not require a CSR. Only modifying jmap would require a CSR. Given the compatibility issues previously mentioned in https://github.com/openjdk/jdk/pull/2261#discussion_r565295921 and subsequent bugs such as [JDK-8219721](https://bugs.openjdk.java.net/browse/JDK-8219721) [JDK-8219896](https://bugs.openjdk.java.net/browse/JDK-8219896), I'm hesitant to add a -parallel option to jmap, as it could lead to similar compatibility issues and bugs. Therefore, the best approach may be to keep it as it is: - For jmap, VM calculates the number of parallel threads (and fallback to serial when unable to parallelize) - For jcmd, VM provides fine-grained control, i.e. allowing -parallel=1-X to control whether to use serial or parallel. Do you think this is acceptable? 2. Test > Running the changes, I got a test failure on macos debug (x64 and aarch64): > test/hotspot/jtreg/serviceability/HeapDump/IntegrityHeapDumpTest.java Fixed. The reason is that `SATestUtils.createProcessBuilder` in IntegrityHeapDump.java may require [user input for a password](https://github.com/y1yang0/jdk/actions/runs/5599168245/job/15188881990#step:9:978) when starting a process. I have modified test to be consistent with DuplicateClassArrayClassesTest.java to avert this problem. 3. Release Note >This enhancement should probably have a release note, and we could briefly >explain what parallel means. > I can help with the wording if needed, if you'd like to start something Thank you in advance! I've created related release-note issue, and I'll draft an initial version for this, please feel free to rephrase it. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13667#issuecomment-1659605530
