8358735: GenShen: block_start() may be incorrect after class unloading #27353

kdnilsen · 2025-09-17T20:12:49Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8358735: GenShen: block_start() may be incorrect after class unloading (Bug - P3)

Reviewers

William Kemper (@earthling-amzn - Reviewer)

Contributors

Y. Srinivas Ramakrishna <ysr@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27353/head:pull/27353
$ git checkout pull/27353

Update a local copy of the PR:
$ git checkout pull/27353
$ git pull https://git.openjdk.org/jdk.git pull/27353/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27353

View PR using the GUI difftool:
$ git pr show -t 27353

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27353.diff

Using Webrev

Link to Webrev Comment

be removed after debugging of a rare crash observed in pipeline testing.

…ock_start to aid better serviceability. Might be placed under #ifdef ASSERT to avoid perff impact in release builds.

assert under adverse conditions that the planned fix is expected to correct.

be fixed before this is ready. In particular, fails reliably with TestClone w/genshen (at least).

…ments

kdnilsen · 2025-09-17T20:14:17Z

/author: ysramakrishna

bridgekeeper · 2025-09-17T20:15:03Z

👋 Welcome back kdnilsen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-09-17T20:15:27Z

@kdnilsen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8358735: GenShen: block_start() may be incorrect after class unloading

Co-authored-by: Y. Srinivas Ramakrishna <ysr@openjdk.org>
Reviewed-by: wkemper

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 151 new commits pushed to the master branch:

c6a8027: 8370154: Update @jls and @JVMS taglets to point to local specs dir
f5eacbe: 8371534: C2: Missed Ideal optimization opportunity with AndL and URShiftL
bbeb6bf: 8371493: Simplify search for AdapterHandlerEntry
... and 148 more: https://git.openjdk.org/jdk/compare/13b3d2fca1af71d0aa9908e19630c2e965dd7134...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-09-17T20:16:14Z

@kdnilsen The following labels will be automatically applied to this pull request:

hotspot-gc
shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

This reverts commit 80198ab.

kdnilsen · 2025-11-06T14:16:42Z

/contributor add @ysramakrishna

openjdk · 2025-11-06T14:17:41Z

@kdnilsen
Contributor Y. Srinivas Ramakrishna <ysr@openjdk.org> successfully added.

ysramakrishna

It looks like a few older comments had not been published previously. I am flushing those and will take a fresh look at the review.

ysramakrishna · 2025-10-03T20:15:16Z

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp

  HeapWord* get_next_marked_addr(const HeapWord* addr,
                                 const HeapWord* limit) const;

+  // 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?

Yeah. The reason for the asymmetry is that forward-looking limit may not be a legitimate address (may be end of heap), whereas backward looking limit is a legitimate address.

ysramakrishna · 2025-10-03T20:16:50Z

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp

+  template<bm_word_t flip, bool aligned_left>
+  inline idx_t get_prev_bit_impl(idx_t l_index, idx_t r_index) const;
+
+  inline idx_t get_next_one_offset(idx_t l_index, idx_t r_index) const;


Please document analogous to line 131.

Sorry. I overlooked this request in prior response. Done.

ysramakrishna · 2025-10-03T20:18:46Z

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp

+  inline idx_t get_next_one_offset(idx_t l_index, idx_t r_index) const;

-  void clear_large_range (idx_t beg, idx_t end);
+  // 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.)

See comment above regarding asymmetry. It is by design, due to shape of the data.

ysramakrishna · 2025-10-03T20:19:44Z

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp

+  // Search for last one in the range [l_index, r_index).  Return r_index if not found.
+  inline idx_t get_prev_one_offset(idx_t l_index, idx_t r_index) const;
+
+  void clear_large_range(idx_t beg, idx_t end);


documentation comment.

Nit:

l_index <-> beg
r_index <-> end

in either comment or formal args to make them mutually consistent.

I've added a comment here as well.

ysramakrishna · 2025-10-03T20:42:01Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp

      // common.
-      HeapWord* p = _scc->block_start(dirty_l);
+      assert(ctx != nullptr || heap->old_generation()->is_parsable(), "Error");
+      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.

ysramakrishna · 2025-10-03T20:47:16Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp

+  // If not null, ctx holds the complete marking context of the old generation. If null,
+  // we expect that the marking context isn't available and the crossing maps are valid.
+  // Note that crossing maps may be invalid following class unloading and before dead
+  // 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.

Thanks for identifying this "confusion". I'm making an attempt to improve documentation for this comment.

ysramakrishna · 2025-10-03T20:49:01Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp

+
+  // if marking context is valid and we are below tams, we use the marking bit map to find the first marked object that
+  // intersects with this card, and if no such object exists, we return null
+  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.

That comment is wrong, which is what caused you to request the alternative semantics for this function. Your comments and questions motivated me to rewrite the comments describing the behavior of this function. Rewriting the comments helped me realize the API was a bit ill-defined. I made some improvements to the behavior so that the definition could be more clearly defined. The new implementation now passes all tests again.

…object_start()

kdnilsen · 2025-11-11T18:38:05Z

/integrate

openjdk · 2025-11-11T18:38:59Z

@kdnilsen This pull request has not yet been marked as ready for integration.

kdnilsen · 2025-11-11T21:06:09Z

/integrate

openjdk · 2025-11-11T21:07:37Z

Going to push as commit 8531fa1.
Since your change was applied there have been 151 commits pushed to the master branch:

c6a8027: 8370154: Update @jls and @JVMS taglets to point to local specs dir
f5eacbe: 8371534: C2: Missed Ideal optimization opportunity with AndL and URShiftL
bbeb6bf: 8371493: Simplify search for AdapterHandlerEntry
... and 148 more: https://git.openjdk.org/jdk/compare/13b3d2fca1af71d0aa9908e19630c2e965dd7134...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-11-11T21:07:46Z

@kdnilsen Pushed as commit 8531fa1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

ysramakrishna

I am still going through this, but it seems as if there's a bunch of potential clean-ups to do here. I realize this has already been integrated; I can may be create a separate task to perhaps clean up some of the questions raised in this.

Since there are quite a few comments now, I am going to flush these for now as a record of some of my thoughts and create a separate task in which I can see if these concerns are real and if the code can be somewhat simplified in a few places.

Nothing specific to do here at this time in response to these stream-of-consciousness comments.

ysramakrishna · 2025-11-10T23:44:14Z

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp

+  // Search for last one in the range [l_index, r_index).  Return r_index if not found.
+  inline idx_t get_prev_one_offset(idx_t l_index, idx_t r_index) const;
+
+  void clear_large_range(idx_t beg, idx_t end);


Nit:

l_index <-> beg
r_index <-> end

in either comment or formal args to make them mutually consistent.

ysramakrishna · 2025-11-11T00:41:18Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp

+      if (end_addr <= left) {
+        // The range of addresses to be scanned is empty
+        continue;
+      }


When would this happen? We start off with dirty_l to the left of dirty_r, and with dirty_r having started at a card that would correspond to end_addr. I am not convinced this check is needed. I'd rather assert here that: assert(left <= end_addr, "left should remain left of end_addr established above");

ysramakrishna · 2025-11-11T01:21:03Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp

+      // on a very large object, i.e. one spanning multiple cards,
+      // if its head card is dirty. If not, (i.e. its head card is clean)
+      // we'll call it each time we process a new dirty range on the object.
+      // This is always the case for large object arrays, which are typically more


Instead of This is always the case ... may be we can say The latter is aways the case ...?

(Mea culpa for the old comment.)

ysramakrishna · 2025-11-11T01:24:52Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp

      // common.
-      HeapWord* p = _scc->block_start(dirty_l);
+      assert(ctx != nullptr || heap->old_generation()->is_parsable(), "Error");
+      HeapWord* p = _scc->first_object_start(dirty_l, ctx, tams, right);


TODO: Wondering if we need to pass both tams and right, or just the max of the two. Will look at first_object_start().

Ah, looks like at this point we might potentially have ctx == nullptr and tams == nullptr. I wonder if we can do better here in terms of passing a sensible single right and dispense with passing tams entirely? Let me go back and look at the implementation of the method again.

ysramakrishna · 2025-11-11T01:35:59Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp

 //     over clusters processed by different workers, with each worked responsible
 //     for scanning the portion of the obj-array overlapping the dirty cards in
 //     its cluster.
 //  3. Non-array objects are precisely dirtied by the interpreter and the compilers


This should say "imprecisely" at line 296, I think?

ysramakrishna · 2025-11-11T02:21:57Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp

  assert(!region->is_humongous(), "Use region->humongous_start_region() instead");
 #endif
+
+  HeapWord* right = MIN2(region->top(), end_range_of_interest);


This is a safe thing to do, but doesn't the caller already establish the invariant that
region->top() >= end_range_of_interest ? Can we just assert that instead of doing the clip/clamp? (And rename the formal parameter name from end_range_of_interest to right?)

If so, we might also want to change the name of the formal parameter from card_index to left, and change it to be a (card-aligned) heap address for symmetry in the API.

ysramakrishna · 2025-11-11T02:25:25Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp

+  HeapWord* end_of_search_next = MIN2(right, tams);
+  size_t last_relevant_card_index;
+  if (end_range_of_interest == _end_of_heap) {
+    last_relevant_card_index = _rs->card_index_for_addr(end_range_of_interest - 1);
+  } else {
+    last_relevant_card_index = _rs->card_index_for_addr(end_range_of_interest);
+    if (_rs->addr_for_card_index(last_relevant_card_index) == end_range_of_interest) {
+      last_relevant_card_index--;
+    }
+  }


I am not sure this is necessary. I'd just adjust the caller so this is ensured, avoiding this computation here. I think the caller has the last dirty card address and can just use that? I realize there's a bit of an issue with the address for tams not necessarily being card-aligned, but I think we should be able to deal with that in the caller as well once we remember that all of this always happens within a single region. (We can add such an assertion so that future adjustments do not render this assumption invalid if the code is changed/adjusted later.)

ysramakrishna · 2025-11-11T22:42:46Z

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp

+#ifdef ASSERT
+  ShenandoahHeap* heap = ShenandoahHeap::heap();
+  ShenandoahHeapRegion* r = heap->heap_region_containing(addr);
+  ShenandoahMarkingContext* ctx = heap->marking_context();
+  HeapWord* tams = ctx->top_at_mark_start(r);
+  assert(limit != nullptr, "limit must not be null");
+  assert(limit >= r->bottom(), "limit must be more than bottom");
+  assert(addr <= tams, "addr must be less than TAMS");
+#endif


Wouldn't it make more sense for these checks to move to the caller? It appears to me to be a leakage of abstraction to test these conditions here. We should be able to return the address for the marked bit found without interpreting the semantics of the bits themselves?

I notice that this is the case for the get_next_... version below as well; if my comment makes some sense, this can be addressed separately.

Perhaps frugality in testing the conditions required us to site these assertions here, which I kind of understand, although the right thing in that case is to have the wrapper class, viz. ShenandoahMarkingContext make those checks before calling here.

ysramakrishna · 2025-11-12T00:37:16Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp

 #endif
+
+  HeapWord* right = MIN2(region->top(), end_range_of_interest);
+  HeapWord* end_of_search_next = MIN2(right, tams);


Does the caller ensure that tams is always valid (e.g. when ctx == nullptr)?

The caller seems to allow for tams==nullptr and ctx==nullptr. In that case wouldn't we get end_of_search_next==nullptr?

ysramakrishna · 2025-11-12T00:40:22Z

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp

-  // Expects to be called for a card affiliated with the old generation in
-  // generational mode.
-  HeapWord* block_start(size_t card_index) const;
+  // Given a card_index, return the starting address of the first live object in the heap


The interface/API comment should describe a Dijkstra-like pre- and post-condition. i.e. if these conditions are satisfied, we'll give you this result.

A description of what the method does (i.e. how it implements the functionality) belongs in the method implementation.

Here, the two are conflated making the interface description unnecessarily long and convoluted. Sometimes this might indicate that the interface isn't as frugal as it should be.

I might state this more succinctly as follows:

// Given: // `card_index`: a valid index of a card in the old generation // `ctx` : a valid marking context for the old generation // `tams` : a valid top-at-mark-start address for the old generation // region in which the card_index is located // `end_range_of_interest` : an address in that region beyond which we need // not locate an object // // Returns: // the address of the object, if any, at the least address that overlaps with // the address range between the start of card_index and end_range_of_interest, // or nullptr if no such object exists in the given range.

Once you look at the spec in this manner, you realize that the first and last arguments go together and define a suitable address range, and the second and third arguments go together and provide a context. This allows you to divide the assertion checking and call interface most optimally between caller and callee.

ysramakrishna and others added 20 commits June 6, 2025 01:48

Fix incorrect loop, tweak asserts.

eec0844

Enable #undef's code for testing

f28571d

Refine fix further

893f5e8

More cleanups.

256944a

More testing; including in product builds. Product mode guarantees will

051e574

be removed after debugging of a rare crash observed in pipeline testing.

Merge branch 'master' into block_start

6c607af

Replace calls to inlined method codes in asserts with class to methods.

0b4eac8

Merge branch 'master' into block_start

f304980

Fix call to static method in assert.

212aed4

May not be checked in; slightly stricter verification of blocks in bl…

b8fff8d

…ock_start to aid better serviceability. Might be placed under #ifdef ASSERT to avoid perff impact in release builds.

first_object_start() doesn't yet do the right thing. Caller should

e2b61ed

assert under adverse conditions that the planned fix is expected to correct.

looking back over a (shenmarking)bitmap. WIP, has (plenty of) bugs.

c1356ef

Stash before proceeding on vacation. This branch has bugs that need to

63fde7e

be fixed before this is ready. In particular, fails reliably with TestClone w/genshen (at least).

Some early efforts on understanding problems with block_start improve…

69452aa

…ments

some progress on get_last_marked_object()

7b48ebf

Add special handling above TAMS

ca728de

Revert extraneous changes

faaf779

Add handling for first_object_start() past end of range

4f1057e

Merge remote-tracking branch 'jdk/master' into finish-block-start

490638f

Remove troublesome assert that assumes lock is held

84ad6b6

kdnilsen marked this pull request as draft September 17, 2025 20:12

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Sep 17, 2025

kdnilsen added 3 commits September 17, 2025 21:27

add explicit typecast to avoid compiler warning message

0583a04

disable for debug build, alphabetic order for includes

7578484

fix white space

9c87c2f

kdnilsen added 4 commits October 20, 2025 22:19

Fixup handling of weakly marked objects in remembered set

80198ab

fix bugs in implementation of weakly referenced object handling

e16ea23

Merge remote-tracking branch 'jdk/master' into finish-block-start

d341522

Revert "Fixup handling of weakly marked objects in remembered set"

643cdfd

This reverts commit 80198ab.

earthling-amzn approved these changes Nov 6, 2025

View reviewed changes

ysramakrishna reviewed Nov 6, 2025

View reviewed changes

kdnilsen changed the title ~~8358735: GenShen: bug in #undef'd code in block_start()~~ 8358735: GenShen: block_start() may be incorrect after class unloading Nov 6, 2025

openjdk bot added the ready Pull request is ready to be integrated label Nov 6, 2025

fix up comments and simplify API for ShenandoahScanRemembered::first_…

637c177

…object_start()

openjdk bot removed the ready Pull request is ready to be integrated label Nov 7, 2025

kdnilsen added 5 commits November 7, 2025 11:20

consider last_relevant_card in determining right-most address

0e2120b

Refinements and debugging

29f5d42

fix multiple errors introduced by minor refactoring of API

cee16f8

Remove debug instrumentation

9f629a2

Add two comments

2dc7e98

earthling-amzn approved these changes Nov 11, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 11, 2025

openjdk bot added the integrated Pull request has been integrated label Nov 11, 2025

openjdk bot closed this Nov 11, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 11, 2025

ysramakrishna reviewed Nov 12, 2025

View reviewed changes

kdnilsen mentioned this pull request Nov 18, 2025

8372110: GenShen: Fix erroneous assert #28375

Closed

3 tasks

8358735: GenShen: block_start() may be incorrect after class unloading #27353

8358735: GenShen: block_start() may be incorrect after class unloading #27353

Uh oh!

Conversation

kdnilsen commented Sep 17, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

kdnilsen commented Sep 17, 2025

Uh oh!

bridgekeeper bot commented Sep 17, 2025

Uh oh!

openjdk bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdnilsen commented Nov 6, 2025

Uh oh!

openjdk bot commented Nov 6, 2025

Uh oh!

ysramakrishna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdnilsen commented Nov 11, 2025

Uh oh!

openjdk bot commented Nov 11, 2025

Uh oh!

kdnilsen commented Nov 11, 2025

Uh oh!

openjdk bot commented Nov 11, 2025

Uh oh!

openjdk bot commented Nov 11, 2025

Uh oh!

ysramakrishna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdnilsen commented Sep 17, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 17, 2025 •

edited

Loading

openjdk bot commented Sep 17, 2025 •

edited

Loading