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

[Thomas Schatzl]

Hi Amit,

On 18.09.20 18:59, Amit Pawar wrote:
> 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
>>
[...]
>>
>> - 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.

Okay.

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

The code tries to avoid virtual memory operation during resizes that 
occur at both ends of the existing committed memory area. Head contains 
work "to the left" of the current committed memory area, tail to the right.

Most often if not all the time one of these areas will be empty (idk 
when there would be resizes at both ends), but let's keep it. Both areas 
can benefit from the pre-touch improvement though.

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