RFR: 8254739: G1: Optimize evacuation failure for regions with few failed objects [v18]

[Thomas Schatzl]

On Wed, 3 Nov 2021 01:09:34 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> This is a try to optimize evcuation failure for regions.
>> I record every evacuation failure object per region (by G1EvacuationFailureObjsInHR), and then iterate (which indeed includes compact/sort/iteration) these objects directly in RemoveSelfForwardPtrHRClosure.
>> 
>> I have tested it with following parameters, 
>> 
>> -   -XX:+ParallelGCThreads=1/32/64
>> -   -XX:G1EvacuationFailureALotInterval=1
>> -   -XX:G1EvacuationFailureALotCount=2/10/100/1000/10000/100000
>> 
>> It saves "Remove Self Forwards" time all the time ,and in most condition it saves "Evacuate Collection Set" time. 
>> 
>> It brings some performance degradation when -XX:G1EvacuationFailureALotCount is low, such as *2*. To improve this a little, we can record the number evacuation failure object per region, and not record these objects when the number hit some limit. But I'm not sure if it's necessary to do so, as I think such condition is so extreme to be met in real environment, although I'm not quite sure.
>
> Hamlin Li has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 19 commits:
> 
>  - Merge branch 'openjdk:master' into speedup-iterate-evac-failure-objs-in-one-region
>  - Refine code based on Thomas' suggestion
>  - Merge branch 'openjdk:master' into speedup-iterate-evac-failure-objs-in-one-region
>  - Fix compilation error
>  - Fix compilation error
>  - Refactor as Thomas suggested
>  - Rename from g1EvacuationFailureObjsInHR to g1EvacFailureObjsInHR
>  - Add asserts, comments
>  - Use refactored G1SegmentedArray rather than home-made Array+Node
>  - Fix wrong merge
>  - ... and 9 more: https://git.openjdk.java.net/jdk/compare/bb92fb02...d33f87b4

I think this is good now - great!

There is one minor comment for some refactoring that you may want to consider.

src/hotspot/share/gc/g1/g1EvacFailureObjectsSet.cpp line 114:

> 112:   void iterate(ObjectClosure* closure) {
> 113:     join_and_sort();
> 114:     iterate_internal(closure);

I would probably move the array allocation and freeing here instead of having this at the start and end of `join_and_sort` and `iterate_internal` respectively. Otherwise there is a hidden dependency on the first method allocating and the second freeing it, and looks cleaner as allocation and deallocation is obvious and on the same call level.

I.e.


  void iterate(ObjectClosure* closure) {
     uint num = _segments->num_allocated_nodes();
    _offset_array = NEW_C_HEAP_ARRAY(OffsetInRegion, num, mtGC);

    join_and_sort();
    iterate_internal(closure);

    FREE_C_HEAP_ARRAY(OffsetInRegion, _offset_array);
  }

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

Marked as reviewed by tschatzl (Reviewer).

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