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

[Stefan Johansson]

Hi Thomas,

> 5 dec. 2019 kl. 14:00 skrev Thomas Schatzl <thomas.schatzl at oracle.com>:
> 
> Hi Stefan,
> 
> On 03.12.19 19:21, Stefan Johansson wrote:> 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/
> >>>[...]
> >> - 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.
> 
> Okay :)
> 
> >
> >> 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.
> >
> 
> Fine with me.
> 
> >> - 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.
> >
> >> - 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.
> >
> 
> Okay.
> 
> >
> > Here are the updated webrevs:
> > Full: http://cr.openjdk.java.net/~sjohanss/8165443/01/
> > Inc: http://cr.openjdk.java.net/~sjohanss/8165443/00-01/
> >
> 
> Some more nits (sorry for not mentioning these in the first review - I was a bit distracted by the placement of the rebuild-free-list code):
> 
No problem =)

> - HeapRegionManager::abandon_free_list should be private, and the implementation could easily be moved to the cpp file.
> Also HRM::append_to_free_list.
> 
> I would probably just manually inline these two one-liner methods though.
I agree, no need for the wrappers now when everything lives in HRM.

> 
> - G1Collectionset::iterate_part_from can be private
Fixed.

> 
> - I think there will be some merge errors with latest changes (i.e. the update to the WorkerDataArray at least)
Yes, I will take care of those when I rebase.

> 
> - heapRegionManager.hpp: please move G1RebuildFreeListTask into the cpp file. It is only ever used there. This allows undo of the changes to the includes in the hpp file too.
Good catch.

> 
> - in G1FreeCollectionSetClosure::handle_evacuated_region, the code could remove the call to clear_remset_locked() and set the skip_remset parameter of the free_region call to false. Since this seems to have been the only case of the skip_remset==true, the parameter can be removed.
> Also the "locked" parameter seems to be always true (now?), so this one can be removed too. The removal of these parameters propagates a bit, but that would reduce complexity of related methods quite a bit :)
I removed the explicit call to clear_remset_locked() and changed the parameter to false. I think this is a good change for this patch.

> 
> Please add an is_at_safepoint() or so call there then.
> 
> I am open to moving this to a separate follow-up RFE, but it does not seem that big of a change, and I think it makes the code easier to understand.
I think the rest of this should be handled separately in follow up, filed: https://bugs.openjdk.java.net/browse/JDK-8235427

> 
> - (this is imho) in the method FreeCSetStats::update_used_before() and
> FreeCSetStats::increment_regions_freed() are always called together I recommend merging them. To me it distracts a bit from the flow of the code with details (given that you already added methods to the FreeCSetStats class). I would also merge the calls to update_failure_used/update_failure_waste/update_used_after in handle_failed_region.
I like this suggestion and I renamed the functions a bit too.

> 
> Looks good otherwise.
Thanks, here are updated webrevs:
Full: http://cr.openjdk.java.net/~sjohanss/8165443/02/
Inc: http://cr.openjdk.java.net/~sjohanss/8165443/01-02/

Thanks,
Stefan

> 
> Thanks,
>  Thomas
>