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

[Thomas Schatzl]

On Sat, 9 Oct 2021 03:53:35 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 incrementally with one additional commit since the last revision:
> 
>   refine code

More unnecessary changes during refactoring I did not notice earlier.

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

> 49:   }
> 50: 
> 51:   uint next_num_elems(uint prev_num_elems) {

use `override` here.

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

> 53:   }
> 54: 
> 55:   uint elem_size () const {return _elem_size;}

This one should be removed, already defined (exactly the same) in the base class.

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

> 125:     return sizeof(*this) +
> 126:       _segmented_array.num_buffers() * sizeof(G1CardSetBuffer)
> 127:             + _segmented_array.num_available_nodes() * _segmented_array.elem_size();

Please keep the operator in the preceeding line; weird indentation.

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

> 131:     return (_segmented_array.num_available_nodes()
> 132:               - (_segmented_array.num_allocated_nodes() - _num_pending_nodes))
> 133:                 * _segmented_array.elem_size();

I prefer to keep the operator in the preceeding line. It is also okay to break through the 80 character line width limit now and then - it's not a very hard limit.

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

> 133:                 * _segmented_array.elem_size();
> 134:   }
> 135:   inline uint num_buffers() { return _segmented_array.num_buffers(); }

missing newline before `num_buffers`

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

> 75:   size_t mem_size() const { return sizeof(*this) + (size_t)_num_elems * _elem_size; }
> 76: 
> 77:   uint length() {

This method is unused in the current code. Please remove.

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

> 86: 
> 87: 
> 88: 

Two extra newlines

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

> 126: class G1SegmentedArrayAllocOptions {
> 127: protected:
> 128:   uint _elem_size;

Newline after visibility specifier.

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

> 132:   uint _alignment;
> 133: 
> 134:   uint exponential_expand(uint prev_num_elems) {

Unused in this class - remove or use the sub-classes' implementation for now.

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

> 148:   }
> 149: 
> 150:   uint next_num_elems(uint prev_num_elems) {

Probably it's best to define this as virtual abstract and override as needed in subclasses. Or just keep the implementation from `G1CardSetAllocOptions` here for now.

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

> 195:   G1SegmentedArrayBuffer<flag>* _last;                 // The last element of the list of all buffers.
> 196:   volatile uint _num_buffers;             // Number of assigned buffers to this allocator.
> 197:   volatile size_t _mem_size;              // Memory used by all buffers.

The comments for the last two were aligned with the ones above in the original.

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

> 208: public:
> 209:   uint num_available_nodes() const { return _num_available_nodes; }
> 210:   uint num_allocated_nodes() const { return _num_allocated_nodes; }

volatile variables should be loaded via `Atomic::load()` (also in `first_array_buffer`)

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

> 210:   uint num_allocated_nodes() const { return _num_allocated_nodes; }
> 211:   const G1SegmentedArrayBuffer<flag>* first_array_buffer() const { return _first; }
> 212:   inline uint elem_size() const;

Please space out these four new getters a little - one before `first_array_buffer`, another before `elem_size` to give them some air.

Also it would be nice to declare these in the same order of the members.

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

Changes requested by tschatzl (Reviewer).

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