RFR: 8252221: Use multiple workers for Parallel GC pre-touching

[Amit Pawar]

On Tue, 15 Sep 2020 13:39:14 GMT, Amit Pawar <github.com+71302734+amitdpawar at openjdk.org> wrote:

> Please review this change.

Hi Thomas,

Thanks for your suggestions and please see my inline reply
> 
> 
> _Mailing list message from [Thomas Schatzl](mailto:thomas.schatzl at oracle.com) on
> [hotspot-gc-dev](mailto:hotspot-gc-dev at openjdk.java.net):_
> Hi Amit,
> 
> thanks for your contribution.
> 
> On 16.09.20 18:18, Amit Pawar wrote:
> 
> > Please review this change.
> 
> ^^--- please consider putting a few lines of explanation in there like
> in a regular (old-style) RFR email. People may respond more eagerly if
> they do not have to search around what you want to accomplish.

OK.

> 
> Thanks.
> 
> - I am not sure why you want some callers of MutableSpace::initialize()
> not use the workgang for parallel pretouch. Particularly after resizing
> the old gen it might be very good to use it.

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.

> 
> Just drop all optional parameters to intialize then, as I would prefer
> to have the Workgang as the last parameter.
> 
> - mutableSpace.cpp:166
> 
> if (pretouch_gang) {
> 
> coding guidelines prohibit relying on implicit conversion of integers to
> booleans. Please add != NULL. See below too.
> 
> - mutableSpace.cpp:170/184:

OK.

> 
> the optimization to not call pretouching code just complicates the code:
> you do not save anything interesting as pretouching takes much longer
> than that check.
> 
> - I am not completely clear why the change only parallelizes the "head"
> portion of the MutableSpace in the initialize() method.
> 
> - afaict the current pretouch code does not pretouch the tail of the
> requested memory range if it is not evenly divisible by #threads and
> page size. Is this intentional?

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.

> 
> - there are some other issues in the new gangtask code, but instead of
> rolling your own pretouch task in this change, please move the code in
> G1PageBasedVirtualSpace::pretouch() (changing parameters to use
> start_addr/end_addr and a page_size parameter in addition to the
> workgang) to a new class with that method as public static in the
> gc/shared directory and use it in both places.
> 
> Remove the "G1" prefixes in the shared code of course.
> 
> That pretouch gang task code one has been tested and if changes are
> required, they do not need to be done in multiple places.

Will do the change as suggested.

> 
> Thanks,
> Thomas

Thanks,
Amit

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

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