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

[Thomas Schatzl]

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

Hi,

> > Sorry for the late reply. Coincidentally around release there's also some housekeeping to do.
> 
> Thanks Thomas, it's OK, this is not that urgent.
> 
> > My main question about this change is: Would it be possible to give a sneak-peek on the use of `G1SegmentedArray` in the [JDK-8254739](https://bugs.openjdk.java.net/browse/JDK-8254739) code? It is impossible to determine if the change fits the intended purpose, and I want to avoid changing `G1SegmentedArray` again later.
> 
> Sure, I've put the merged code in a temporary branch, https://github.com/Hamlin-Li/jdk/tree/tmp-merge.segmented-array.speed-remove-self. "speed-remove-self" part is still the old one, I did not change it yet, including the suggestions you gave in #5181 and old home made segmented array implementation; I just use the new G1SegmentedArray in G1EvacuationFailureObjsInHR.

Thanks, that helped a lot to understand the change. I did some preliminary clean-up of then unused code, and it looks much nicer then.

> 
> > This means that multiple threads might add new segments to it at the same time, meaning that they may try to append new segments to it. What happens with the thread and the segment that fails? Shouldn't this allocation be reused somehow (maybe for other segmented arrays?).
> 
> Current(original) implementation just drop the newly allocated G1SegmentedArrayBuffer if current thread fails, seems it's fine as the race should be rare, should we add the faield one to free list? seems adding to freelist will bring some benefit.

Yes, that's also what `G1CardSetAllocator` does. At least there, the race isn't that rare.

> 
> > Also, this seems to drop the concurrent freeing of these segments after GC, which is nice too.
> > The remembered set uses the memory provided by the segmented arrays in chunks too, i.e. the `G1CardSetArray` container. Not sure if something like this would be more appropriate here instead of directly using the chunks (but may well be as good).
> 
> Not quite sure, seems both have benefit, in `G1CardSetArray` way, it has better design benefit; in G1SegmentedArray::iterate_nodes/G1EvacuationFailureObjsInHR::visit, it might have better performance. I'm OK with both ways, please kindly let me know your decision.

The concurrent freeing of the arrays is a good to have since the infrastructure to do it is already there. It can be added later though.

Thanks,
  Thomas

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

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