RFR: 8374780: G1: Do not iterate klass metadata when splitting objArrays for every slice

[Thomas Schatzl]

On Fri, 9 Jan 2026 09:37:47 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this change that makes concurrent mark not try to iterate over `objArray` metadata for every slice, to make behavior uniform with other garbage collection types/garbage collectors.
>> 
>> Testing: tier1-5
>> 
>> Thanks,
>>   Thomas
>
> src/hotspot/share/gc/g1/g1ConcurrentMark.inline.hpp line 192:
> 
>> 190:   int start = checked_cast<int>(pointer_delta(MAX2(mr.start(), obj->base()), obj->base(), heapOopSize));
>> 191:   int end = checked_cast<int>(pointer_delta(mr.end(), obj->base(), heapOopSize));
>> 192:   obj->oop_iterate_range(_cm_oop_closure, start, end);
> 
> I think this becomes a bit obscure when the lines perform multiple tasks, and having one line per operation makes it easier to make sure that the code is correct.
> 
> If we look at the operations we also see an inconsistency between `start` and `end` calculation:
> 
> For `start` it calculates the lower boundary and then it converts to an index.
> For `end` it *skips* calculating a upper boundary and then it converts to an index.
> 
> You need to read the callers to figure out if skipping calculating the upper boundary for `end` is correct.
> 
> I would strongly suggest that this code is rewritten so that `scan_objArray` takes a `start` and `end` as array indices, instead of using a `MemRegion`. And that the same is done for `process_array_slice`. I think then some of these casts and `pointer_deltas` will go away and the code will be easier to read and reason about.

I think most of the complexity you correctly point out comes from the unusual way G1 concurrent mark handles object array splitting.

To avoid having two queues (e.g. one for objArrayOops, one for other oops), and then dealing with the question and complexity which to prefer to work on, it encodes objArrayOops as pointers somewhere into that oop.

Everything else follows - the `MemRegion` passed to this method can be any part of the object, that may include the header. It needs to be skipped when calling the existing API to just iterate over array elements as it takes indexes.

On hindsight I agree splitting out the MAX2() would have been better though.

I will wait with this change on @walulyai 's effort to have g1 concurrent mark use the `PartialArraySplitter` used everywhere else, which will afaik naturally use indexes and all this conversion going away. Should have done that in the first place :I

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

PR Review Comment: https://git.openjdk.org/jdk/pull/29116#discussion_r2681599476