RFR: 8165443: Free Collection Set serial phase takes very long on large heaps

[Stefan Johansson]

Hi Thomas,

Thanks for taking a look.

On 2019-11-29 11:55, Thomas Schatzl wrote:
> Hi,
> On 26.11.19 16:01, Stefan Johansson wrote:
>> Hi,
>> 
>> Please review this fix to improve freeing the collection set when having a lot of regions.
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8165443
>> Webrev: http://cr.openjdk.java.net/~sjohanss/8165443/00/
>> 
>   some initial comments:
> - g1CollectionSet::iterate_part_from: s/lenght/length
Oups, fixed.

> - HeapRegion::handle_evacuation_failed() -> handle_evacuation_failure()
Fixed.

> - heapRegionManager.hpp:176: s/region/regions
Fixed.

> - could the FreeRegionList refactoring be factored out? Not insisting on this, but might decrease the webrev quite a bit >
> - G1FreeCollectionSetTask::G1FreeCollectionSetClosure::EventForRegion: maybe rename to JFREventForRegion to make it more clear that this is for JFR events.
Changed.

> - I would prefer that if a change modifies timing code, move it to use Ticks internally, i.e. in case of ...::TimerForRegion use Ticks/Tickspan as time base, not double, to slowly move to using Ticks everywhere.
> Same with the double's for _young_time and _non_young_time in the area.
> Also G1CollectedHeap::free_collection_set() should be changed imho.
> (Yes, this introduces some ugly Tickspan::seconds() * 1000.0 calls, but they are probably easier to clean up later).
Ok, changed to use Ticks.

> - there is imho no need for the G1FreeCollectionSetClosure::stats() getter as it is only ever used locally and is trivial. Please at least make it private :)
Made it private, I prefer the look of the code when using the getter in this case.

> - not sure how I feel about not calling the destructor for the worker's FreeCSetStats. While it is empty I would still recommend calling it before freeing the containing array.
> - same with the local FreRegionLists in ~G1RebuildFreeListTask.
Added calls to the destructors.

> - HeapRegionManager::is_available() is (mostly) meant as internal function, but due to assert'ing I was forced to make it public (probably should have a non-public version and a public one that is only available with assertions).
> The use in G1RebuildFreeListTask::work() kind of violates this idea (and sorry for not mentioning it anywhere in the code).
This was just a very small optimization instead of using at_or_null(), so reverted back to use that one.

> Maybe move the entire freelist rebuild task into HeapRegionManager where it imho fits much better? HeapRegionManager is and should be the "owner" of the free list.
> The call to HeapRegion::unlink_from_list() can probably be made earlier, or see below.
> - another change I do not really like is the difference between "abandoning" the free list (and then later clearing the HeapRegion links in parallel) and "removing" the free list. While it makes sense from a performance POV, I would be happier if we could get away without introducing this tiny semantic difference.
> An option would be that there were a FreeRegionList::add_to_tail that just overwrites the links. This would remove the need for the new "abandon" and other support methods in a few places and hide the ugliness there.
> What do you think?
Discussed this a bit with Thomas offline and we both agree that there can be further cleanups improvements here, but I think we agreed on just moving stuff into the HeapRegionManager but keep the added concepts. We both agree that having both remove_all and abandon for the freelist is not good in the long run, but getting rid of remove_all or fix remove_all to perform well is out of scope for this enhancement.

> - while you mentioned that you did not look into balancing the work for the rebuild free list action, but please limit the workers in the G1RebuildFreeListTask by at least the number of regions in the heap.
> (Move the chunk sizing outside of the G1RebuildFreeListTask for that.)
Added a clamp to make sure we never use more workers than we have regions, even though in most cases this should never happen.

> This sounds comical, but current machines' number of available threads are quickly approaching small heap sizes...
> - instead of workers()->run_task() please use G1CollectedHeap::run_task(). That also removes some timing code for you around these places :)
Can’t use that one since I need to time the destructor of the task as well.

> - in the log, I believe that the "(Non-)Young Free collection set" "phases" should be indented one more place.
> I.e.
> GC(0)     Free Collection Set: 0.0ms
> GC(0)       Serial Free Collection Set: 0.0ms
> GC(0)       Parallel Free Collection Set (ms): Min: ...
> GC(0)       Young Free Collection Set (ms): Min:  ...
> GC(0)       Non-Young Free Collection Set (ms): skipped
> should be
> GC(0)     Free Collection Set: 0.0ms
> GC(0)       Serial Free Collection Set: 0.0ms
> GC(0)       Parallel Free Collection Set (ms): Min: ...
> GC(0)         Young Free Collection Set (ms): Min:  ...
> GC(0)         Non-Young Free Collection Set (ms): skipped
Totally agree, added a extra_indent argument similar to what debug_phase already had.

Here are the updated webrevs:
Full: http://cr.openjdk.java.net/~sjohanss/8165443/01/
Inc: http://cr.openjdk.java.net/~sjohanss/8165443/00-01/

Thanks,
Stefan

> imo.
> Thanks,
>   Thomas