RFR: 8254167 G1: Record regions where evacuation failed to provide targeted iteration [v4]

[Thomas Schatzl]

On Tue, 31 Aug 2021 14:37:44 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> This is another try to optimize evcuation failure for regions.
>> I record evacuation failed regions, and iterate these regions directly rather than iterate the whole cset.
>> The implementation reuses and refactors some of the existing data in g1CollectedHeap (_regions_failed_evacuation, _num_regions_failed_evacuation), and records these regions in an array, and iterate this array later in post evacuation phase.
>> 
>> I have 2 implementations:
>> 
>> - 1) CHT (Initial version)
>> - 2) bitmap (latest version, reuse _regions_failed_evacuation in g1CollectedHeap)
>> 
>> This implementation does not consider work distribution as mentioned in JDK-8254167 yet. But seems it already get better&stable performance gain than origin. We could improve it further later if work distribution is necessary.
>> 
>> I will attach the perf data in JBS.
>
> Hamlin Li has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - merge from master
>  - Fix missing header files
>  - reuse original bitmap rather than CHT
>  - merge with existing classes
>  - Initial commit

Please also remove the ".g1CollectedHeap.hpp.swp" file accidentally committed.

> This implementation does not consider work distribution as mentioned in JDK-8254167 yet. But seems it already get better&stable performance gain than origin. We could improve it further later if work distribution is necessary.

I'll split out this request into a new CR to not forget; because while this is a good step forward, it would be good to know the actual number of objects (or bytes, or whatever) that failed evacuation to better distribute work. That can be added though when tackling that issue.

Overall, it looks like what I expected 👍

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 881:

> 879:   void preserve_mark_during_evac_failure(uint worker_id, oop obj, markWord m);
> 880: 
> 881:   void iterate_evacuation_failure_regions_par(HeapRegionClosure* closure,

Please avoid additional clutter of `G1CollectedHeap`. Pass `_evac_failure_regions` to `post_evacuate_collection_set` and then down to `G1ParRemoveSelfForwardPtrsTask` where it calls `par_iterate` directly.
I am currently moving stuff, particularly data structures only used in the young collector to a separate class (JDK-8253343). So while it is not avoidable to put `_evac_failure_regions` here, let's avoid further adding young-collector only stuff to `G1CollectedHeap`.

I'll of course do those changes (i.e. moving code to `G1YoungCollector`) and further related cleanup when ready.

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

> 275:   // otherwise unreachable object at the very end of the collection. That object
> 276:   // might cause an evacuation failure in any region in the collection set.
> 277:   // _g1h->collection_set_par_iterate_all(&rsfp_cl, &_hrclaimer, worker_id);

Please remove this debug code. Also the comment above is obsolete: now that we know in which regions exactly the evacuation failure happened, this comment does not apply any more.

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.cpp line 1:

> 1: /*

It's a bit weird that all other similar files start with "g1EvacFailure" and this one with "g1EvacuationFailure" - please change according to existing style (or rename all others first in a separate CR).

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.cpp line 26:

> 24: 
> 25: #include "precompiled.hpp"
> 26: #include "g1EvacuationFailureRegions.hpp"

should be `gc/g1/g1Evacu....`

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.cpp line 27:

> 25: #include "precompiled.hpp"
> 26: #include "g1EvacuationFailureRegions.hpp"
> 27: #include "gc/g1/g1CollectedHeap.hpp"

Includes should be sorted alphabetically.

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.cpp line 28:

> 26: #include "g1EvacuationFailureRegions.hpp"
> 27: #include "gc/g1/g1CollectedHeap.hpp"
> 28: #include "gc/g1/g1CollectedHeap.inline.hpp"

There is no need to include both `.inline.hpp` and `.hpp`.

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.cpp line 38:

> 36: 
> 37: G1EvacuationFailureRegions::~G1EvacuationFailureRegions() {
> 38:   FREE_C_HEAP_ARRAY(uint, _evac_failure_regions);

The include for `FREE_C_HEAP_ARRAY` is missing.

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.cpp line 43:

> 41: void G1EvacuationFailureRegions::initialize() {
> 42:   Atomic::store(&_evac_failure_regions_cur_length, 0u);
> 43:   _max_regions = G1CollectedHeap::heap()->max_reserved_regions();

Please pass in `max_reserved_regions()` - this makes the dependency on it during initialization obvious.

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

> 49:                                              HeapRegionClaimer* _hrclaimer,
> 50:                                              uint worker_id) {
> 51:   assert_at_safepoint();

As far as I can see this is a verbatim copy of `G1CollectionSet::iterate_part_from` (with some minor simplifications). Not really happy about that, need to think more about how/if we can avoid this.

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.hpp line 35:

> 33: 
> 34: // This class
> 35: //     1. records for every region on the heap whether evacuation failed for it.

Please write about the class without using a list; at most use an unordered list as there seems to be no ordering here (i.e. at most use bullets). I strongly prefer just two regular sentences.

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.hpp line 40:

> 38: class G1EvacuationFailureRegions {
> 39: 
> 40: private:

unnecessary `private` and newline.

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.hpp line 41:

> 39: 
> 40: private:
> 41:   CHeapBitMap _regions_failed_evacuation;

Please keep the existing comments (from `G1CollectedHeap`).

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

> 49:   void initialize();
> 50:   void reset();
> 51:   bool contains(uint region_idx) const;

Please add a newline between initialization/reset stuff and other methods.

src/hotspot/share/gc/g1/g1EvacuationFailureRegions.hpp line 72:

> 70: };
> 71: 
> 72: 

Superfluous newline

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

Changes requested by tschatzl (Reviewer).

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