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

[Amit Pawar]

On Fri, 25 Sep 2020 12:25:23 GMT, Amit Pawar <github.com+71302734+amitdpawar at openjdk.org> wrote:

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

Thanks for agreeing and will update the PR with suggested changes for final review.

Thanks,
Amit

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

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