RFR: 8273626: G1: Factor out concurrent segmented array from G1CardSetAllocator [v3]

Fri Oct 8 09:37:18 UTC 2021

On Fri, 24 Sep 2021 08:37:30 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> To finish https://bugs.openjdk.java.net/browse/JDK-8254739, we need a segmented array to store a growing regions index array, in the initial version of that patch, a newly home made segmented array was used, but the memory efficiency is not as good as expected, G1CardSetAllocator is a potential candidate to fullfill the requirement, but need some enhancement.
>> 
>> This is a try to enhance G1CardSetAllocator(and G1CardSetBuffer, G1CardSetBufferList) to support element size less pointer size, and strip this basic function as a more generic segmented array (G1SegmentedArray).
>
> Hamlin Li has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into generalize-g1CardSetBuffer-and-Allocator
>  - Rename Xxx to G1Xxx
>  - Clean code
>  - Fix wrong length() in SegmentedArrayBuffer, cause it might grow more than _elem_nums
>  - Initial commit

I think other than the given comments the change seems good, but there will be a second thorough path.

Also the direct use of `G1SegmentedArrayBuffer` (I will *likely* suggest a separate `G1SegmentedArray` subclass for the new features) in `G1EvacuationFailureObjsInHR` seems okay.

src/hotspot/share/gc/g1/g1CardSetMemory.cpp line 88:

> 86: void G1CardSetAllocator<Elem>::free(Elem* elem) {
> 87:   assert(elem != nullptr, "precondition");
> 88:   // assert(elem_size() >= sizeof(G1CardSetContainer), "size mismatch");

Should be removed as it has been added to the constructor.

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

> 39: // Collects G1CardSetAllocator options/heuristics. Called by G1CardSetAllocator
> 40: // to determine the next size of the allocated G1CardSetBuffer.
> 41: 

Superfluous newline

src/hotspot/share/gc/g1/g1CardSetMemory.hpp line 99:

> 97: // memory.
> 98: template <class Elem>
> 99: class G1CardSetAllocator : public G1SegmentedArray<Elem, mtGCCardSet> {

I think a `G1CardSetAllocator` *has* or *uses* a `G1SegmentedArray`, but itself is not. I.e. this does not seem to be a proper is-a relation, so the subclassing here seems wrong.
Adding a member of that type and using it seems better to me.

src/hotspot/share/gc/g1/g1CardSetMemory.hpp line 143:

> 141: 
> 142:   size_t wasted_mem_size() const {
> 143:     return (G1SegmentedArray<Elem, mtGCCardSet>::num_available_nodes()

Maybe the code looks a bit better with a typedef for `G1SegmentedArray<Elem, mtGCCardSet>`.

src/hotspot/share/gc/g1/g1SegmentedArray.hpp line 84:

> 82:   }
> 83: 
> 84:   const char* start() const { return _buffer; }

I would prefer a `copy_to` method instead of exposing the raw buffer pointer; this is the only use in the follow-up code. That method is in this case better added in that change to highlight it.

src/hotspot/share/gc/g1/g1SegmentedArray.hpp line 146:

> 144: 
> 145:   G1SegmentedArrayAllocOptions(uint elem_size, uint initial_num_elems = MinimumBufferSize,
> 146:                                uint max_num_elems = MaximumBufferSize, uint alignment = BufferAlignment) :

Now that this class is going to be used in multiple contexts, I would prefer if the defaults (and default values) would be removed and added at the place of use.
Just add an empty constructor that sets this to bogus values (zeros probably), and check that these are set in eg. `next_num_elems`.

Alternatively delay this suggested change to the next and change back the value of `BufferAlignment` which I'd actually prefer.

src/hotspot/share/gc/g1/g1SegmentedArray.hpp line 157:

> 155:   }
> 156: 
> 157:   uint elem_size() const {return _elem_size;}

Pre-existing: space after the `{`.

src/hotspot/share/gc/g1/g1SegmentedArray.hpp line 166:

> 164: // G1SegmentedArrayBufferList is the free list to cache G1SegmentedArrayBuffer,
> 165: // and G1SegmentedArrayAllocOptions is the configuration for G1SegmentedArray
> 166: // attributes.

A large part of the `G1CardSetAllocator` documentation needs to go here, talking about MT behavior, how to use etc.

src/hotspot/share/gc/g1/g1SegmentedArray.hpp line 174:

> 172: 
> 173:   volatile uint _num_available_nodes; // Number of nodes available in all buffers (allocated + free + pending + not yet used).
> 174:   volatile uint _num_allocated_nodes; // Number of total nodes allocated and in use.

Since these are purely statistics members I would prefer if they were located last in the member list as they were before.
Typically, order members by "importance", which is of course subjective; but in this case there is no reason to change the order from original code.

src/hotspot/share/gc/g1/g1SegmentedArray.hpp line 213:

> 211:   template<typename Visitor>
> 212:   void iterate_nodes(Visitor& v);
> 213: };

These seem to be new. Please introduce them in the next change. These introductions of new functionality when factoring out code makes it really hard to review (or at least harder than necessary).
The reason I'm saying this is because such additions makes at least me suspicious of what else has been added and changed, resulting in extra effort to look through *everything* again - particularly if the author did not mention that this or that has been added (beyond what is necessary to make things work again).
Which in turn makes reviews like this less appealing to do (they are typically boring - we probably prefer "boring" reviews :) ) because of the obvious extra effort.

src/hotspot/share/gc/g1/g1SegmentedArray.hpp line 215:

> 213: };
> 214: 
> 215: 

Extra newline.

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

> 30: #include "runtime/atomic.hpp"
> 31: #include "utilities/globalCounter.hpp"
> 32: #include "utilities/globalCounter.inline.hpp"

Only the .inline.hpp is needed here.

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

> 61: 
> 62: 
> 63: // ==== G1SegmentedArrayBufferList ====

Please remove these class markers (`//===== <class name> ====`). They are not used anywhere else in gc code but maybe in ancient code and they do not add anything.

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

> 128: template <class Elem, MEMFLAGS flag>
> 129: G1SegmentedArrayBuffer<flag>* G1SegmentedArray<Elem, flag>::create_new_buffer(
> 130:   G1SegmentedArrayBuffer<flag>* const prev) {

I think it is still okay to put the parameter on the previous line.

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

> 220: template <class Elem, MEMFLAGS flag>
> 221: Elem* G1SegmentedArray<Elem, flag>::allocate() {
> 222:   G1SegmentedArrayBuffer<flag>* cur = Atomic::load_acquire(&_first);

>From the original code the `assert(elem_size > 0, ...` is missing and still seems appropriate to be in the base class. The subclass seems to just do that.

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

Changes requested by tschatzl (Reviewer).

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