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

[Hamlin Li]

On Fri, 8 Oct 2021 08:37:10 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> 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
>
> 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.

delete this code in this change, will add it in next change.

> 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.

delete this code in this change, will add it in next change.

> 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.

Seems it''s too long for me, but modified as you suggested.

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

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