RFR: 8358735: GenShen: bug in #undef'd code in block_start() [v2]

[Y. Srinivas Ramakrishna]

On Fri, 3 Oct 2025 00:21:11 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> When scanning a range of dirty cards within the GenShen remembered set, we need to find the object that spans the beginning of the left-most dirty card.  The existing code is not reliable following class unloading.
>> 
>> The new code uses the marking context when it is available to determine the location of live objects that reside below TAMS within each region.  Above TAMS, all objects are presumed live and parsable.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix idiosyncratic formatting

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp line 129:

> 127:   inline idx_t get_prev_bit_impl(idx_t l_index, idx_t r_index) const;
> 128: 
> 129:   inline idx_t get_next_one_offset(idx_t l_index, idx_t r_index) const;

Please document analogous to line 131.

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp line 131:

> 129:   inline idx_t get_next_one_offset(idx_t l_index, idx_t r_index) const;
> 130: 
> 131:   // Search for last one in the range [l_index, r_index).  Return r_index if not found.

Symmetry arguments wrt spec for `get_next_one_offset` may have preferred range `(l_index, r_index]`, returning `l_index` if none found. May be its (transitive) usage prefers this shape? (See similar comment at line 180.)

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp line 134:

> 132:   inline idx_t get_prev_one_offset(idx_t l_index, idx_t r_index) const;
> 133: 
> 134:   void clear_large_range(idx_t beg, idx_t end);

documentation comment.

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp line 180:

> 178:                                  const HeapWord* limit) const;
> 179: 
> 180:   // Return the last marked address in the range [limit, addr], or addr+1 if none found.

Symmetry would have preferred `(limit, addr]` as the range with `limit` if none found.
However, may be usage of this method prefers the present shape?

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 251:

> 249:   // if marking context is valid and we are below tams, we use the marking bit map to find the first marked object that
> 250:   // intersects with this card, and if no such object exists, we return null
> 251:   if ((ctx != nullptr) && (left < tams)) {

It seems like the caller should check if `left >= tams` and short-circuit rather than have this method do that work.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 663:

> 661:   // we expect that the marking context isn't available and the crossing maps are valid.
> 662:   // Note that crossing maps may be invalid following class unloading and before dead
> 663:   // or unloaded objects have been coalesced and filled (updating the crossing maps).

Good comment!

What's still not clear is why `tams` and `last_relevant_card_index` are passed here. Does it reduce the work in the caller? I'd expect this to just return the first object on the card index or null if no such object exists. I realize `ctx` is used when one must consult the marking context in preference to the "crossing maps". The relevance of the last 2 arguments isn't clear from this documentation comment.

May be I'll see why these are passed in when I look at the method definition, but I suspect there may be some leakage of abstraction & functionality here between caller and callee.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp line 177:

> 175:       // common.
> 176:       assert(ctx != nullptr || heap->old_generation()->is_parsable(), "Error");
> 177:       HeapWord* p = _scc->first_object_start(dirty_l, ctx, tams, dirty_r);

Passing `ctx`, `tams`, and `dirty_r` into this method seems interesting. Let's see how they are used.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2403250259
PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2403253570
PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2403255239
PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2403247074
PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2403311952
PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2403309508
PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2403300158