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

[Thomas Schatzl]

Hi again,

   also, I am tempted to suggest to remove the update-methods for the 
internal FreeCSetStats class. Since they are all single-use and very 
local, I would not be against removing them and making the members public.

Thanks,
   Thomas

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