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

[Thomas Schatzl]

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

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

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

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

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

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

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

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

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?

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

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

- 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

imo.

Thanks,
   Thomas