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

[Amit Pawar]

On Fri, 18 Sep 2020 16:56:42 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

Hi Thomas,

Thanks for replying again and will do as per your suggestion.

Regarding using multiple threads to pre-touch old gen post resize.
Initial debugging showed that GC threads do reach here to initialize old gen memory post resizing. This limits to
single threaded only as GC thread is already executing a task and needs special handling. Please see generated log
messages below that included thread name.

2.116s][debug][gc   ] GC(9) Expanding ParOldGen thread name VM Thread from 5632K by 3584K to 9216K
2.249s][debug][gc   ] GC(12) Expanding ParOldGen thread name VM Thread from 9216K by 6144K to 15360K
5.768s][debug][gc   ] GC(33) Expanding ParOldGen thread name GC Thread#139 from 15360K by 512K to 15872K
5.769s][debug][gc   ] GC(33) Expanding ParOldGen thread name GC Thread#92 from 15872K by 512K to 16384K
5.769s][debug][gc   ] GC(33) Expanding ParOldGen thread name GC Thread#13 from 16384K by 512K to 16896K

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

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

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