New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8252221: Use multiple workers for Parallel GC pre-touching #180
Conversation
Hi @amitdpawar, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user amitdpawar" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
@amitdpawar The following label will be automatically applied to this pull request: When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the |
/signed |
Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
Webrevs
|
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi Amit, thanks for your contribution. On 16.09.20 18:18, Amit Pawar wrote:
^^--- please consider putting a few lines of explanation in there like Thanks. - I am not sure why you want some callers of MutableSpace::initialize() Just drop all optional parameters to intialize then, as I would prefer - mutableSpace.cpp:166 if (pretouch_gang) { coding guidelines prohibit relying on implicit conversion of integers to - mutableSpace.cpp:170/184: the optimization to not call pretouching code just complicates the code: - I am not completely clear why the change only parallelizes the "head" - afaict the current pretouch code does not pretouch the tail of the - there are some other issues in the new gangtask code, but instead of Remove the "G1" prefixes in the shared code of course. That pretouch gang task code one has been tested and if changes are Thanks, |
Hi Thomas, Thanks for your suggestions and please see my inline reply
OK.
It does not work correctly for old gen after resizing. One of the tier1 test is running for a long time and not sure why ? Initially thought to handle the startup case first and then resizing next based on feedback. I will come back with more info on this.
OK.
Currently with single thread, pretouch is done separately for both "head" and "tail" part. I didnt get this why ? so thought of following the same and then can be changed based on feedback. I guess both "head" and "tail" can be merged. Please suggest.
Will do the change as suggested.
Thanks, |
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi Amit, On 18.09.20 18:59, Amit Pawar wrote:
[...]
Okay.
The code tries to avoid virtual memory operation during resizes that Most often if not all the time one of these areas will be empty (idk
Thanks, |
Hi Thomas, Thanks for replying again and will do as per your suggestion. Regarding using multiple threads to pre-touch old gen post resize. 2.116s][debug][gc ] GC(9) Expanding ParOldGen thread name VM Thread from 5632K by 3584K to 9216K I would like to fix this in two separate patches. 1st Patch: Current PR fixes JVM startup time only. Pre-touching post resize do it multi-threaded with VM thread and single threaded with non VM threads. This will be a partial fix but not complete. Complete fix can be done in second patch. 2nd Patch: Please check log messages from 33th GC. Old gen size was changed three times in the same GC by three different GC threads. This makes separate Pre-touching needs to be done post every resize. In the second patch this can be done once by combining after all the resizes. I am interested in fixing this but need more time. Please suggest. Thanks, |
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi, On 25.09.20 14:27, Amit Pawar wrote:
So basically the threads themselves do expansion themselves during gc if
This looks like expansion of old gen during promotion. I did not think I do not think this can be delayed as the thread is simply out of Let's at least split this out into a separate change then as you Thanks, |
Thanks for agreeing and will update the PR with suggested changes for final review. Thanks, |
Hi Thomas, PR is updated with suggested changes. Please review. Thanks, |
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi Amit, On 06.10.20 20:14, Amit Pawar wrote:
looks really good now, some minor nits: - g1PageBasedVirtualSpace.cpp/mutableSpace.cpp: please keep the include - mutableSpace.cpp:118+ PretouchTask::pretouch("ParallelGC PreTouch", (char*)head.start(), PretouchTask::pretouch("ParallelGC PreTouch", (char*)tail.start(), - Please move the end_address parameter to the previous line even if - pretouch.hpp: s/2018/2020 in the header; maybe update the end dates of I'll let it pass through testing over the weekend. I will report back if Thanks, |
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi again, some more nits: - the code does not compiler without precompiled headers because of hotspot/share/gc/parallel/mutableSpace.hpp:90:27: error: 'WorkGang' has Please fix and check by building with --disable-precompiled_headers Same in mutableNUMASpace.hpp. The cpp files also need the workgroup.hpp include. - please rename pretouch.hpp to pretouchTask.hpp. - includes are completely missing in pretouch.hpp. Needs at least - please move the implementation of the various methods of PretouchTask Thanks, On 09.10.20 12:53, Thomas Schatzl wrote:
|
Hi Thomas, I have fixed the build issue and also updated the PR with suggested changes. Thanks for testing and please check now. Thanks, |
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi Amit, On 09.10.20 19:08, Amit Pawar wrote:
thanks for your changes. Some more, final minor nits: - mutableSpace.cpp:115: please remove the newline after the "if" - psYoungGen.cpp:641: I do not think PSYoungGen::resize_spaces() can be I.e. ultimately by either PSParallelCompact::invoke_no_policy() or Same with the code in PSYoungGen::reset_survivors_after_shrink(). (I did not actually test this change). - preTouchTask.hpp: please move all implementation to the cpp file, then - preTouchTask.hpp: please drop the first "private:" in the PretouchTask - preTouchTask.hpp: please move the "size_t page_size" for the static - preTouchTask.hpp:56: please drop the extra spaces between function - preTouchTask.cpp: please add "#include "precompiled.hpp"" as first Other than that testing tiers 1-5 seems good so far (70% done) after Thanks, |
Hi Thomas, Thanks for the your feedback and testing and good to hear that it passed 70%. PR is updated as per your suggestions and please check. I have another suggestion regarding "PreTouchParallelChunkSize" flag. ParallelGC = 0.5-1 sec. Also, the minimum value allowed is '1' and it should be similar to page size right ? Please check. Thanks, |
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi, On 13.10.20 15:53, Amit Pawar wrote:
I filed https://bugs.openjdk.java.net/browse/JDK-8254699, although
It should be at least a page size. We can change the CR as required.
Thanks, |
Sorry for the confusion and here is how I tested Command: G1GC Test ParallelGC Test I hope this helps. Thanks, |
Thanks for the suggestions and PR is updated with suggested changes. Thanks, |
lgtm. Thanks. Please wait on Kim's approval and I'll do a final testing round - I think you need somebody sponsoring this change anyway. I can do that.
@amitdpawar This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 419 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kimbarrett, @tschatzl) but any other Committer may sponsor as well.
|
Yes. I also changed it so that you need to wait for a second reviewer (i.e. Kim to be satisfied) before you can do that. It would be a bit rude to not wait until Kim okay'ed it after he started the review. /reviewers 2 |
@tschatzl |
Thanks Kim and Thomas for approving the PR. |
/integrate |
@amitdpawar |
Another round of tier1-5 testing seems to be good. /sponsor |
Thanks @amitdpawar for your contribution! :) |
@tschatzl @amitdpawar Since your change was applied there have been 423 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 9359ff0. |
Please review this change.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/180/head:pull/180
$ git checkout pull/180