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

Fri Oct 29 10:10:24 UTC 2021

On Fri, 29 Oct 2021 06:41:37 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:
> 
>   Fix compilation error

Changes requested by tschatzl (Reviewer).

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

> 49: }
> 50: 
> 51: G1EvacFailureObjectsSet::OffsetInRegion G1EvacFailureObjectsSet::cast_to_offset(oop obj) const {

This should probably be `to_offset` to match `from_offset` (forgot that) - this is not a cast to me (changing the interpretation of a bit pattern) but a transformation (changing the bit pattern for storage savings).

So `cast` seems to be the wrong word to me here.

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

> 52:   const HeapWord* o = cast_from_oop<const HeapWord*>(obj);
> 53:   size_t offset = pointer_delta(o, _bottom);
> 54:   assert_is_valid_offset(offset);

The `from_offset` call below already does this assert.

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

> 60:   DEBUG_ONLY(_region_idx(region_idx) COMMA)
> 61:   _bottom(bottom),
> 62:   _offsets("", &_alloc_options, &_free_buffer_list)  {

Fwiw, I recently removed the first parameter of `G1SegmentedArray`. Please make sure you merge with tip before pushing.

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

> 73: // Helper class to join, sort and iterate over the previously collected segmented
> 74: // array of objects that failed evacuation.
> 75: class G1EvacFailureObjectsIterator {

Note that this is not an `Iterator` in C++ STL sense, so it is a good idea to not name it like that. I understand that Hotspot code (and even the recent remembered set code - there is a CR to fix that) adds to the confusion, but it would be nice to not add to the confusion.

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

> 76:   typedef G1EvacFailureObjectsSet::OffsetInRegion OffsetInRegion;
> 77:   friend class G1SegmentedArray<OffsetInRegion, mtGC>;
> 78:   friend class G1SegmentedArrayBuffer<mtGC>;

I prefer to make `visit_*` public here instead of the friends. This is an internal class not visible outside, and we actually use it as a closure.

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

> 78:   friend class G1SegmentedArrayBuffer<mtGC>;
> 79: 
> 80:   G1EvacFailureObjectsSet* _collector;

Probably rename `_collector` to `_objects_set` or so to match the type better.

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

> 99:   void iterate_internal(ObjectClosure* closure) {
> 100:     for (uint i = 0; i < _array_length; i++) {
> 101:       _collector->assert_is_valid_offset(_offset_array[i]);

This assert duplicates the one in `from_offset` below. Please remove.

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

> 123:     _collector->assert_is_valid_offset(*ptr);
> 124:   }
> 125: #endif

I intentionally removed this `visit_elem` method in the original suggestion because I thought that to be too much checking. It is kind of obvious that we only store data in here.
We check that these are valid offsets both when writing to and reading from the array, so this seems to be unnecessary triple-checking.

Fwiw, in Hotspot code we do not use the `visit_` prefix but `do_` (and pass something that ends with a `Closure`, not a `Visitor` in the `iterate*` methods).
I am aware that in design pattern lingo this is the Visitor pattern, but Hotspot is probably much older than that and it is somewhat jarring to have some code use this terminology and others use another one.

A change here needs to be discussed with a wider audience.

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

> 132:     _array_length(0) { }
> 133: 
> 134:   ~G1EvacFailureObjectsIterator() { }

Please remove the empty unnecessary destructor.

src/hotspot/share/gc/g1/g1SegmentedArray.inline.hpp line 66:

> 64:   }
> 65: }
> 66: 

As mentioned above, I do not think this one is necessary at this point.

src/hotspot/share/gc/g1/heapRegion.cpp line 113:

> 111: void HeapRegion::record_evac_failure_obj(oop obj) {
> 112:   _evac_failure_objs.record(obj);
> 113: }

This should probably be placed in the `.inline.hpp` file as it is called in performance sensitive code.

src/hotspot/share/gc/g1/heapRegion.cpp line 117:

> 115: void HeapRegion::iterate_evac_failure_objs(ObjectClosure* closure) {
> 116:   _evac_failure_objs.iterate(closure);
> 117: }

(This one is fine to be placed in the cpp file - the additional single call per `HeapRegion` does not matter)

src/hotspot/share/gc/g1/heapRegion.hpp line 560:

> 558:   // Update the region state after a failed evacuation.
> 559:   void handle_evacuation_failure();
> 560:   // Records evac failure objs during evaucation, this will help speed up iteration

s/evucation/evacuation.

Please try to avoid "evac failure objs" in text as it seems strange to me grammatically, and isn't particularly obvious what this is to readers not working on this all the time. Just say "Record an object that failed evacuation within this region.` I do not think the second part of the sentence is necessary, i.e. talking about speeding up something here.

src/hotspot/share/gc/g1/heapRegion.hpp line 563:

> 561:   // of these objs later in *remove self forward* phase of post evacuation.
> 562:   void record_evac_failure_obj(oop obj);
> 563:   // Iterates evac failure objs which are recorded during evcauation.

s/evauation/evacuation

Better would probably be "Applies the given closure to all previously recorded objects that failed evacuation in ascending address order"

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

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