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

[Thomas Schatzl]

On Tue, 26 Oct 2021 04:47:59 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 incrementally with one additional commit since the last revision:
> 
>   Rename from g1EvacuationFailureObjsInHR to g1EvacFailureObjsInHR

There are quite a few comments from me about this change, and at some point I decided to provide a change that would cover these and more suggestions I had (note that this change might contradict some of the other suggestions - sometimes this happens when actually tinkering and seeing the whole code); communicating via text is too cumbersome. The commit containing the direction I think the change should go is available at https://github.com/openjdk/jdk/commit/b793f5bfb9e15f938fefe2e64b16eb56818bc695 .

Mostly it is about hiding the code for preparing and iterating the sorted list of objects that failed evacuation.

Maybe `G1EvacFailureObjsInHR` should be renamed to something like `G1EvacFailedObjectsSet` (I do not think the `HR` postfix is important).

As for the other question, for now I think it is good to keep the `G1EvacFailureObjsInHR` in `HeapRegion`.

Please have a look.

src/hotspot/share/gc/g1/g1EvacFailure.cpp line 73:

> 71:     assert(_hr->is_in(obj_addr), "sanity");
> 72: 
> 73:     // The object failed to move.

One could add an assert that `_last_forwarded_object <= obj`. This was implicit in the object walk earlier, but not any more and is only guaranteed by the object sorting.

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.cpp line 35:

> 33: 
> 34: const uint G1EvacFailureObjsInHR::MaxBufferLength =
> 35:   static_cast<uint>(1u << (HeapRegion::LogOfHRGrainBytes-LogHeapWordSize));

I am not sure this `static_cast` is necessary: shifting an `uint` gives an uint, but I did not check...

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.cpp line 84:

> 82:   for (uint i = 0; i < _objs_num; i++) {
> 83:     assert(i == 0 ? (prev <= _offset_array[i]) : (prev < _offset_array[i]),
> 84:            "must be, %u, %u, %u", i, prev, _offset_array[i]);

As suggested, I would move this check that addresses passed to `do_object` must be ascending into `do_object`.

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.cpp line 86:

> 84:            "must be, %u, %u, %u", i, prev, _offset_array[i]);
> 85:     assert(prev < _max_offset, "must be, %u", prev);
> 86:     closure->do_object(cast_from_offset(prev = _offset_array[i]));

Please avoid assignments within unrelated statements :) I almost overlooked that. I.e. just move `prev` one line above.

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.cpp line 118:

> 116:   compact();
> 117:   sort();
> 118:   iterate_internal(closure);

I would prefer if all these methods would not operate on the (temporarily used) member `_offset_array`. I would prefer if this method kept this offset array local and passed it through these methods - that would make the code much more obvious to follow.
Also then it would be obvious that `_offset_array` is actually local, with allocation and deallocation placed here. It's not obvious that `compact` not only compacts the segmented array, but also allocates it. Maybe a better name for `compact` would be `join_segments` too.

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.hpp line 39:

> 37: 
> 38: public:
> 39:   typedef uint Elem;

I am not sure if this indirection is necessary; if so, document what it is though. And why an `uint` is sufficient.
Maybe use something more descriptive than `Elem`, e.g. `OopOffsetInRegion`.

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.hpp line 48:

> 46:   static const G1SegmentedArrayAllocOptions _alloc_options;
> 47:   // This free list is shared among evacuation failure process in all regions.
> 48:   static G1SegmentedArrayBufferList<mtGC> _free_buffer_list;

If not already done, please file an issue that the `_free_buffer_list` is never (partially) reclaimed.

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.hpp line 51:

> 49: 
> 50:   const Elem _max_offset;
> 51:   const uint _region_idx;

Both `_max_offset` and `_region_idx` are only used in verification code. Please make `DEBUG_ONLY`.
`_max_offset` could be replaced by a method recalculating the value since we are talking about a simple shift.

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.hpp line 64:

> 62:     return cast_to_oop(_bottom + offset);
> 63:   }
> 64:   Elem cast_from_oop_addr(oop obj) {

Missing newline.

src/hotspot/share/gc/g1/g1EvacFailureObjsInHR.hpp line 90:

> 88: 
> 89:   // Verify elements in the buffer
> 90:   void visit_elem(void* elem);

Please name this `verify_elems` or so.

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

Changes requested by tschatzl (Reviewer).

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