From eec0844cac3bc8182182ddc13235b3637c5020b1 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Fri, 6 Jun 2025 01:48:35 +0000 Subject: [PATCH 01/32] Fix incorrect loop, tweak asserts. --- src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index f581e8ef9eaa5..a66922a210b65 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -282,8 +282,9 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { NOT_PRODUCT(obj = cast_to_oop(p);) assert(oopDesc::is_oop(obj), "Should be an object"); #define WALK_FORWARD_IN_BLOCK_START false - while (WALK_FORWARD_IN_BLOCK_START && p + obj->size() < left) { + while (WALK_FORWARD_IN_BLOCK_START && p + obj->size() <= left) { p += obj->size(); + obj = cast_to_oop(p); } #undef WALK_FORWARD_IN_BLOCK_START // false assert(p + obj->size() > left, "obj should end after left"); From f28571dbc8981a8714ee5a1c240450ea41c005de Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Fri, 6 Jun 2025 02:51:30 +0000 Subject: [PATCH 02/32] Enable #undef's code for testing --- src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index a66922a210b65..f8af463e462a0 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -281,7 +281,7 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { // 4. Every allocation under TAMS updates the object start array. NOT_PRODUCT(obj = cast_to_oop(p);) assert(oopDesc::is_oop(obj), "Should be an object"); -#define WALK_FORWARD_IN_BLOCK_START false +#define WALK_FORWARD_IN_BLOCK_START true while (WALK_FORWARD_IN_BLOCK_START && p + obj->size() <= left) { p += obj->size(); obj = cast_to_oop(p); From 893f5e82a81cffbaa57247f0dcd00d259f162109 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Fri, 6 Jun 2025 03:29:30 +0000 Subject: [PATCH 03/32] Refine fix further --- .../share/gc/shenandoah/shenandoahScanRemembered.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index f8af463e462a0..42b3b9750d821 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -282,12 +282,14 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { NOT_PRODUCT(obj = cast_to_oop(p);) assert(oopDesc::is_oop(obj), "Should be an object"); #define WALK_FORWARD_IN_BLOCK_START true - while (WALK_FORWARD_IN_BLOCK_START && p + obj->size() <= left) { + while (WALK_FORWARD_IN_BLOCK_START && p + obj->size() < left) { p += obj->size(); obj = cast_to_oop(p); + assert(oopDesc::is_oop(obj), "Should be an object"); } #undef WALK_FORWARD_IN_BLOCK_START // false - assert(p + obj->size() > left, "obj should end after left"); + assert(p <= left, "p should start at or before left end of card"); + assert(p + obj->size() > left, "obj should end after left end of card"); return p; } From 256944a81a4b78cd442840ffbb3b40366518286c Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Fri, 6 Jun 2025 03:44:13 +0000 Subject: [PATCH 04/32] More cleanups. --- .../share/gc/shenandoah/shenandoahScanRemembered.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 42b3b9750d821..daa1731ee0e77 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -245,8 +245,6 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { return left; } - HeapWord* p = nullptr; - oop obj = cast_to_oop(p); ssize_t cur_index = (ssize_t)card_index; assert(cur_index >= 0, "Overflow"); assert(cur_index > 0, "Should have returned above"); @@ -262,7 +260,7 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { assert(starts_object(cur_index), "Error"); size_t offset = get_last_start(cur_index); // can avoid call via card size arithmetic below instead - p = _rs->addr_for_card_index(cur_index) + offset; + HeapWord* p = _rs->addr_for_card_index(cur_index) + offset; // Recall that we already dealt with the co-initial object case above assert(p < left, "obj should start before left"); // While it is safe to ask an object its size in the loop that @@ -279,7 +277,7 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { // evacuation phase) of young collections. This is never called // during old or global collections. // 4. Every allocation under TAMS updates the object start array. - NOT_PRODUCT(obj = cast_to_oop(p);) + oop obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Should be an object"); #define WALK_FORWARD_IN_BLOCK_START true while (WALK_FORWARD_IN_BLOCK_START && p + obj->size() < left) { From 051e57468b35dac901b6e63f8105ef1155c793c4 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Tue, 10 Jun 2025 00:22:11 +0000 Subject: [PATCH 05/32] More testing; including in product builds. Product mode guarantees will be removed after debugging of a rare crash observed in pipeline testing. --- .../shenandoah/shenandoahScanRemembered.cpp | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index daa1731ee0e77..41964cd51257c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -265,29 +265,38 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { assert(p < left, "obj should start before left"); // While it is safe to ask an object its size in the loop that // follows, the (ifdef'd out) loop should never be needed. - // 1. we ask this question only for regions in the old generation + // 1. we ask this question only for regions in the old generation, and those + // that are not humongous regions // 2. there is no direct allocation ever by mutators in old generation - // regions. Only GC will ever allocate in old regions, and then - // too only during promotion/evacuation phases. Thus there is no danger + // regions walked by this code. Only GC will ever allocate in old regions, + // and then too only during promotion/evacuation phases. Thus there is no danger // of races between reading from and writing to the object start array, // or of asking partially initialized objects their size (in the loop below). + // Furthermore, humongous regions (and their dirty cards) are never processed + // by this code. // 3. only GC asks this question during phases when it is not concurrently // evacuating/promoting, viz. during concurrent root scanning (before // the evacuation phase) and during concurrent update refs (after the // evacuation phase) of young collections. This is never called - // during old or global collections. + // during global collections during marking or update refs.. // 4. Every allocation under TAMS updates the object start array. oop obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Should be an object"); +#ifdef ASSERT +#define WALK_FORWARD_IN_BLOCK_START true +#else #define WALK_FORWARD_IN_BLOCK_START true +#endif // ASSERT while (WALK_FORWARD_IN_BLOCK_START && p + obj->size() < left) { p += obj->size(); obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Should be an object"); + // Check assumptions in previous block comment if this assert fires + guarantee(false, "Should never need forward walk in block start"); } -#undef WALK_FORWARD_IN_BLOCK_START // false - assert(p <= left, "p should start at or before left end of card"); - assert(p + obj->size() > left, "obj should end after left end of card"); +#undef WALK_FORWARD_IN_BLOCK_START + guarantee(p <= left, "p should start at or before left end of card"); + guarantee(p + obj->size() > left, "obj should end after left end of card"); return p; } From 0b4eac86a043317dd22d67cea12f15757ef8d8e1 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Wed, 13 Aug 2025 20:40:57 +0000 Subject: [PATCH 06/32] Replace calls to inlined method codes in asserts with class to methods. --- src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index ee85b92f1188e..9989b796b5fcd 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -125,9 +125,8 @@ void ShenandoahDirectCardMarkRememberedSet::mark_read_table_as_clean() { // No lock required because arguments align with card boundaries. void ShenandoahCardCluster::reset_object_range(HeapWord* from, HeapWord* to) { - assert(((((unsigned long long) from) & (CardTable::card_size() - 1)) == 0) && - ((((unsigned long long) to) & (CardTable::card_size() - 1)) == 0), - "reset_object_range bounds must align with card boundaries"); + assert(is_card_aligned(from) && is_card_aligned(to), + "Must align with card boundaries"); size_t card_at_start = _rs->card_index_for_addr(from); size_t num_cards = (to - from) / CardTable::card_size_in_words(); From 212aed4c4883e3b0a70306453905c40a66cb9174 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Wed, 13 Aug 2025 21:21:23 +0000 Subject: [PATCH 07/32] Fix call to static method in assert. --- src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 9989b796b5fcd..a002ee6378c61 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -125,7 +125,7 @@ void ShenandoahDirectCardMarkRememberedSet::mark_read_table_as_clean() { // No lock required because arguments align with card boundaries. void ShenandoahCardCluster::reset_object_range(HeapWord* from, HeapWord* to) { - assert(is_card_aligned(from) && is_card_aligned(to), + assert(CardTable::is_card_aligned(from) && CardTable::is_card_aligned(to), "Must align with card boundaries"); size_t card_at_start = _rs->card_index_for_addr(from); size_t num_cards = (to - from) / CardTable::card_size_in_words(); From b8fff8d23268dfb66a2b41e3d215ab63f4b33911 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Tue, 19 Aug 2025 18:33:44 +0000 Subject: [PATCH 08/32] May not be checked in; slightly stricter verification of blocks in block_start to aid better serviceability. Might be placed under #ifdef ASSERT to avoid perff impact in release builds. --- src/hotspot/share/gc/shared/markBitMap.hpp | 12 ++++++++++-- src/hotspot/share/gc/shared/markBitMap.inline.hpp | 15 ++++++++++++++- .../gc/shenandoah/shenandoahScanRemembered.cpp | 2 ++ src/hotspot/share/oops/klass.cpp | 9 ++++++++- 4 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/hotspot/share/gc/shared/markBitMap.hpp b/src/hotspot/share/gc/shared/markBitMap.hpp index a55a8220910ae..237b289489244 100644 --- a/src/hotspot/share/gc/shared/markBitMap.hpp +++ b/src/hotspot/share/gc/shared/markBitMap.hpp @@ -79,11 +79,19 @@ class MarkBitMap { } // Return the address corresponding to the next marked bit at or after - // "addr", and before "limit", if "limit" is non-null. If there is no - // such bit, returns "limit" if that is non-null, or else "endWord()". + // "addr", and before "limit", which are required to be non-null. In other + // words, we look for a marked address in the range [addr, limit). If there is no + // such bit, returns "limit". inline HeapWord* get_next_marked_addr(const HeapWord* addr, HeapWord* limit) const; + // Return the address corresponding to the last marked bit before + // "addr" but at or after "limit", which are required to be non-null. + // In other words, we look for a marked address in the range [limit, addr). + // If there is no such bit, returns "addr". + inline HeapWord* get_last_marked_addr(HeapWord* limit, + const HeapWord* addr) const; + void print_on(outputStream* st, const char* prefix) const; // Write marks. diff --git a/src/hotspot/share/gc/shared/markBitMap.inline.hpp b/src/hotspot/share/gc/shared/markBitMap.inline.hpp index cf6cf02cf320c..3dfe4ce0142b3 100644 --- a/src/hotspot/share/gc/shared/markBitMap.inline.hpp +++ b/src/hotspot/share/gc/shared/markBitMap.inline.hpp @@ -36,13 +36,26 @@ inline HeapWord* MarkBitMap::get_next_marked_addr(const HeapWord* const addr, HeapWord* const limit) const { assert(limit != nullptr, "limit must not be null"); - // Round addr up to a possible object boundary to be safe. + assert(addr != nullptr, "addr must not be null"); + // Round up to bitmap's alignment boundary to start looking size_t const addr_offset = addr_to_offset(align_up(addr, HeapWordSize << _shifter)); size_t const limit_offset = addr_to_offset(limit); size_t const nextOffset = _bm.find_first_set_bit(addr_offset, limit_offset); return offset_to_addr(nextOffset); } +inline HeapWord* MarkBitMap::get_last_marked_addr(HeapWord* const limit, + const HeapWord* const addr) const { + assert(limit != nullptr, "limit must not be null"); + assert(addr != nullptr, "addr must not be null"); + size_t const limit_offset = addr_to_offset(limit); + // Round down to bitmap's alignment boundary to start looking back + size_t const addr_offset = addr_to_offset(align_down(addr, HeapWordSize << _shifter)); + size_t const lastOffset = _bm.find_last_set_bit(limit_offset, addr_offset); + return offset_to_addr(lastOffset); +} + + inline void MarkBitMap::mark(HeapWord* addr) { check_mark(addr); _bm.set_bit(addr_to_offset(addr)); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index a002ee6378c61..3d4d76332edbd 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -281,6 +281,7 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { // 4. Every allocation under TAMS updates the object start array. oop obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Should be an object"); + assert(Klass::is_valid(obj->klass()), "Not a valid klass ptr"); #ifdef ASSERT #define WALK_FORWARD_IN_BLOCK_START true #else @@ -290,6 +291,7 @@ HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { p += obj->size(); obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Should be an object"); + assert(Klass::is_valid(obj->klass()), "Not a valid klass ptr"); // Check assumptions in previous block comment if this assert fires guarantee(false, "Should never need forward walk in block start"); } diff --git a/src/hotspot/share/oops/klass.cpp b/src/hotspot/share/oops/klass.cpp index 41ab8e325ab7c..169a07a1cacb4 100644 --- a/src/hotspot/share/oops/klass.cpp +++ b/src/hotspot/share/oops/klass.cpp @@ -1110,7 +1110,14 @@ bool Klass::is_valid(Klass* k) { if (!Metaspace::contains(k)) return false; if (!Symbol::is_valid(k->name())) return false; - return ClassLoaderDataGraph::is_valid(k->class_loader_data()); + + // YSR_DEBUGGING + if (SafepointSynchronize::is_at_safepoint()) { + return ClassLoaderDataGraph::is_valid(k->class_loader_data()); + } else { + MutexLocker x(ClassLoaderDataGraph_lock, Mutex::_no_safepoint_check_flag); + return ClassLoaderDataGraph::is_valid(k->class_loader_data()); + } } Method* Klass::method_at_vtable(int index) { From e2b61ed070adb37ea5a0b9d72ec699f55a4b6e32 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Wed, 20 Aug 2025 03:04:58 +0000 Subject: [PATCH 09/32] first_object_start() doesn't yet do the right thing. Caller should assert under adverse conditions that the planned fix is expected to correct. --- .../gc/shenandoah/shenandoahScanRemembered.cpp | 17 ++++++++++++----- .../gc/shenandoah/shenandoahScanRemembered.hpp | 18 ++++++++++++------ .../shenandoahScanRemembered.inline.hpp | 15 +++++++++------ 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 3d4d76332edbd..9dd4b95913504 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -221,11 +221,18 @@ size_t ShenandoahCardCluster::get_last_start(size_t card_index) const { return _object_starts[card_index].offsets.last; } -// Given a card_index, return the starting address of the first block in the heap -// that straddles into this card. If this card is co-initial with an object, then -// this would return the first address of the range that this card covers, which is -// where the card's first object also begins. -HeapWord* ShenandoahCardCluster::block_start(const size_t card_index) const { +// Given a card_index, return the starting address of the first object in the heap +// that intersects with this card. This must be a valid, parsable object, and must +// be the first such object that intersects with this card. The object may start before, +// at, or after the start of the card, and may end in or after the card. If no +// such object exists, a null value is returned. +// Expects to be called for a card in an region affiliated with the old generation in +// generational heap, otherwise behavior is undefined. +// 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). +HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, const ShenandoahMarkingContext* const ctx) const { HeapWord* left = _rs->addr_for_card_index(card_index); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp index 5df34159c0ff9..19940f05bcdca 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp @@ -650,12 +650,18 @@ class ShenandoahCardCluster: public CHeapObj { size_t get_last_start(size_t card_index) const; - // Given a card_index, return the starting address of the first block in the heap - // that straddles into the card. If the card is co-initial with an object, then - // this would return the starting address of the heap that this card covers. - // 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 object in the heap + // that intersects with this card. This must be a valid, parsable object, and must + // be the first such object that intersects with this card. The object may start before, + // at, or after the start of the card, and may end in or after the card. If no + // such object exists, a null value is returned. + // Expects to be called for a card in an region affiliated with the old generation in + // generational heap, otherwise behavior is undefined. + // 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). + HeapWord* first_object_start(size_t card_index, const ShenandoahMarkingContext* const ctx) const; }; // ShenandoahScanRemembered is a concrete class representing the diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp index 68bec5c2071bc..651349fd01169 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp @@ -167,14 +167,17 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou assert(right <= region->top() && end_addr <= region->top(), "Busted bounds"); const MemRegion mr(left, right); - // NOTE: We'll not call block_start() repeatedly - // on a very large object if its head card is dirty. If not, - // (i.e. the 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 + // NOTE: We'll not call first_object_start() repeatedly + // 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 // 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); oop obj = cast_to_oop(p); + assert(oopDesc::is_oop(obj), "Not an object"); + assert(ctx==nullptr || ctx->is_marked(obj), "Error"); // PREFIX: The object that straddles into this range of dirty cards // from the left may be subject to special treatment unless From c1356efdeeeb33a05e09c847040bbbe92fbd5c75 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Wed, 20 Aug 2025 17:57:14 +0000 Subject: [PATCH 10/32] looking back over a (shenmarking)bitmap. WIP, has (plenty of) bugs. --- .../gc/shenandoah/shenandoahMarkBitMap.cpp | 19 ++++++ .../gc/shenandoah/shenandoahMarkBitMap.hpp | 19 +++++- .../shenandoahMarkBitMap.inline.hpp | 67 +++++++++++++++++++ .../shenandoah/shenandoahMarkingContext.hpp | 3 + .../shenandoahMarkingContext.inline.hpp | 4 ++ .../shenandoah/shenandoahScanRemembered.cpp | 40 ++++++++++- 6 files changed, 148 insertions(+), 4 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp index 34e6af41b427c..401ecdc1476e4 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp @@ -58,6 +58,25 @@ bool ShenandoahMarkBitMap::is_bitmap_clear_range(const HeapWord* start, const He } +HeapWord* ShenandoahMarkBitMap::get_last_marked_addr(const HeapWord* limit, + const HeapWord* addr) const { +#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 + + // Round addr up to a possible object boundary to be safe. + size_t const addr_offset = address_to_index(align_down(addr, HeapWordSize << LogMinObjAlignment)); + size_t const limit_offset = address_to_index(limit); + size_t const nextOffset = get_last_one_offset(limit_offset, addr_offset); + return index_to_address(nextOffset); +} + HeapWord* ShenandoahMarkBitMap::get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const { #ifdef ASSERT diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp index 56daf4c595671..faa7abb531090 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp @@ -119,7 +119,16 @@ class ShenandoahMarkBitMap { template inline idx_t get_next_bit_impl(idx_t l_index, idx_t r_index) const; + // Helper for get_last_{zero,one}_bit variants. + // - flip designates whether searching for 1s or 0s. Must be one of + // find_{zeros,ones}_flip. + // - aligned_left is true if l_index is a priori on a bm_word_t boundary. + template + inline idx_t get_last_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; + inline idx_t get_last_one_offset (idx_t l_index, idx_t r_index) const; void clear_large_range (idx_t beg, idx_t end); @@ -162,12 +171,16 @@ class ShenandoahMarkBitMap { bool is_bitmap_clear_range(const HeapWord* start, const HeapWord* end) const; - // Return the address corresponding to the next marked bit at or after - // "addr", and before "limit", if "limit" is non-null. If there is no - // such bit, returns "limit" if that is non-null, or else "endWord()". + // Return the first marked address in the range [addr, limit), or limit + // if none found. HeapWord* get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const; + // Return the last marked address in the range (limit, addr], or limit + // if none found. + HeapWord* get_last_marked_addr(const HeapWord* limit, + const HeapWord* addr) const; + bm_word_t inverted_bit_mask_for_range(idx_t beg, idx_t end) const; void clear_range_within_word (idx_t beg, idx_t end); void clear_range (idx_t beg, idx_t end); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp index f0a9752b614c4..44c716c501312 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp @@ -171,10 +171,77 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_bit_impl(idx_t return r_index; } +template +inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_last_bit_impl(idx_t l_index, idx_t r_index) const { + STATIC_ASSERT(flip == find_ones_flip || flip == find_zeros_flip); + verify_range(l_index, r_index); + assert(!aligned_left || is_aligned(l_index, BitsPerWord), "l_index not aligned"); + + // The first word often contains an interesting bit, either due to + // density or because of features of the calling algorithm. So it's + // important to examine that first word with a minimum of fuss, + // minimizing setup time for later words that will be wasted if the + // first word is indeed interesting. + + // The benefit from aligned_left being true is relatively small. + // It saves an operation in the setup for the word search loop. + // It also eliminates the range check on the final result. + // However, callers often have a comparison with l_index, and + // inlining often allows the two comparisons to be combined; it is + // important when !aligned_left that return paths either return + // l_index or a value dominating a comparison with l_index. + // aligned_left is still helpful when the caller doesn't have a + // range check because features of the calling algorithm guarantee + // an interesting bit will be present. + + if (l_index < r_index) { + // Get the word containing r_index, and shift out low bits. + idx_t index = to_words_align_down(r_index); + bm_word_t cword = (map(index) ^ flip) >> bit_in_word(r_index); + if ((cword & 1) != 0) { + // The first bit is similarly often interesting. When it matters + // (density or features of the calling algorithm make it likely + // the first bit is set), going straight to the next clause compares + // poorly with doing this check first; count_trailing_zeros can be + // relatively expensive, plus there is the additional range check. + // But when the first bit isn't set, the cost of having tested for + // it is relatively small compared to the rest of the search. + return r_index; + } else if (cword != 0) { + // Flipped and shifted first word is non-zero. + idx_t result = r_index + count_trailing_zeros(cword); + if (aligned_left || (result > l_index)) return result; + // Result is beyond range bound; return l_index. + } else { + // Flipped and shifted first word is zero. Word search through + // aligned up r_index for a non-zero flipped word. + idx_t limit = aligned_left + ? to_words_align_down(l_index) // Minuscule savings when aligned. + : to_words_align_up(l_index); + while (--index > limit) { + cword = map(index) ^ flip; + if (cword != 0) { + idx_t result = bit_index(index) + count_trailing_zeros(cword); + if (aligned_left || (result > l_index)) return result; + // Result is beyond range bound; return r_index. + assert((index - 1) == limit, "invariant"); + break; + } + } + // No bits in range; return l_index. + } + } + return l_index; +} + inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_one_offset(idx_t l_offset, idx_t r_offset) const { return get_next_bit_impl(l_offset, r_offset); } +inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_last_one_offset(idx_t l_offset, idx_t r_offset) const { + return get_last_bit_impl(l_offset, r_offset); +} + // Returns a bit mask for a range of bits [beg, end) within a single word. Each // bit in the mask is 0 if the bit is in the range, 1 if not in the range. The // returned mask can be used directly to clear the range, or inverted to set the diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp index 8a52042e513ef..1710f34bbd77f 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp @@ -67,7 +67,10 @@ class ShenandoahMarkingContext : public CHeapObj { inline bool is_marked_or_old(oop obj) const; inline bool is_marked_strong_or_old(oop obj) const; + // get the first marked address in the range [addr,linit), or limit if none found inline HeapWord* get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const; + // get the last marked address in the range (limit, addr), or limit if none found + inline HeapWord* get_last_marked_addr(const HeapWord* limit, const HeapWord* addr) const; inline bool allocated_after_mark_start(const oop obj) const; inline bool allocated_after_mark_start(const HeapWord* addr) const; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp index e3ba774283c18..db7ee03046fb5 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp @@ -72,6 +72,10 @@ inline HeapWord* ShenandoahMarkingContext::get_next_marked_addr(const HeapWord* return _mark_bit_map.get_next_marked_addr(start, limit); } +inline HeapWord* ShenandoahMarkingContext::get_last_marked_addr(const HeapWord* limit, const HeapWord* start) const { + return _mark_bit_map.get_last_marked_addr(limit, start); +} + inline bool ShenandoahMarkingContext::allocated_after_mark_start(oop obj) const { const HeapWord* addr = cast_from_oop(obj); return allocated_after_mark_start(addr); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 9dd4b95913504..b378584982805 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -235,15 +235,53 @@ size_t ShenandoahCardCluster::get_last_start(size_t card_index) const { HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, const ShenandoahMarkingContext* const ctx) const { HeapWord* left = _rs->addr_for_card_index(card_index); + ShenandoahHeapRegion* region = ShenandoahHeap::heap()->heap_region_containing(left); #ifdef ASSERT assert(ShenandoahHeap::heap()->mode()->is_generational(), "Do not use in non-generational mode"); - ShenandoahHeapRegion* region = ShenandoahHeap::heap()->heap_region_containing(left); assert(region->is_old(), "Do not use for young regions"); // For HumongousRegion:s it's more efficient to jump directly to the // start region. assert(!region->is_humongous(), "Use region->humongous_start_region() instead"); #endif + + // if marking context is valid, 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) { + if (ctx->is_marked(left)) { + oop obj = cast_to_oop(left); + assert(oopDesc::is_oop(obj), "Should be an object"); + return left; + } + // get the previous marked object, if any + if (region->bottom() < left) { + HeapWord* prev = ctx->get_last_marked_addr(region->bottom(), left); + if (prev != nullptr) { + oop obj = cast_to_oop(prev); + assert(oopDesc::is_oop(obj), "Should be an object"); + HeapWord* obj_end = prev + obj->size(); + if (obj_end > left) { + return prev; + } + } + } + // the prev marked object, if any, ends before left; + // find the next marked object if any on this card + assert(!ctx->is_marked(left), "Was dealt with above"); + HeapWord* right = MIN2(region->top(), ctx->top_at_mark_start(region)); + HeapWord* next = ctx->get_next_marked_addr(left, right); + if (next < right) { + oop obj = cast_to_oop(next); + assert(oopDesc::is_oop(obj), "Should be an object"); + } + return next; + } + assert(ctx == nullptr, "Should have returned above"); + + // The following code assumes that the region has only parsable objects + assert(ShenandoahGenerationalHeap::heap()->old_generation()->is_parsable(), + "The code that follows expects a parsable heap"); if (starts_object(card_index) && get_first_start(card_index) == 0) { // This card contains a co-initial object; a fortiori, it covers // also the case of a card being the first in a region. From 63fde7e2015ab5ec0f8294a09a61937af2ffcbd6 Mon Sep 17 00:00:00 2001 From: Ramki Ramakrishna Date: Sat, 6 Sep 2025 09:27:28 +0000 Subject: [PATCH 11/32] Stash before proceeding on vacation. This branch has bugs that need to be fixed before this is ready. In particular, fails reliably with TestClone w/genshen (at least). --- .../shenandoah/shenandoahScanRemembered.cpp | 2 ++ .../gc/shenandoah/compiler/TestClone.java | 27 +++---------------- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index b378584982805..6fbc9c0e1abd1 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -270,6 +270,7 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // find the next marked object if any on this card assert(!ctx->is_marked(left), "Was dealt with above"); HeapWord* right = MIN2(region->top(), ctx->top_at_mark_start(region)); + assert(right > left, "We don't expect to be examining cards above the smaller of TAMS or top"); HeapWord* next = ctx->get_next_marked_addr(left, right); if (next < right) { oop obj = cast_to_oop(next); @@ -277,6 +278,7 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } return next; } + assert(ctx == nullptr, "Should have returned above"); // The following code assumes that the region has only parsable objects diff --git a/test/hotspot/jtreg/gc/shenandoah/compiler/TestClone.java b/test/hotspot/jtreg/gc/shenandoah/compiler/TestClone.java index 2e98c72ee176b..ae297e5d4312c 100644 --- a/test/hotspot/jtreg/gc/shenandoah/compiler/TestClone.java +++ b/test/hotspot/jtreg/gc/shenandoah/compiler/TestClone.java @@ -331,32 +331,11 @@ * -XX:-UseCompressedOops * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational * -XX:+ShenandoahVerify - * TestClone - * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xms1g -Xmx1g - * -XX:-UseCompressedOops - * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational - * -XX:+ShenandoahVerify - * -Xint - * TestClone - * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xms1g -Xmx1g - * -XX:-UseCompressedOops - * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational - * -XX:+ShenandoahVerify - * -XX:-TieredCompilation - * TestClone - * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xms1g -Xmx1g - * -XX:-UseCompressedOops - * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational - * -XX:+ShenandoahVerify - * -XX:TieredStopAtLevel=1 - * TestClone - * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xms1g -Xmx1g - * -XX:-UseCompressedOops - * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational - * -XX:+ShenandoahVerify - * -XX:TieredStopAtLevel=4 + * -Xlog:gc*=info:file=/home/ysr/gc_ysr.log + * -XX:+ShowMessageBoxOnError -XX:+UseCompactObjectHeaders * TestClone */ + public class TestClone { public static void main(String[] args) throws Exception { From 69452aa3fb1a2503dd0ddf9190442b996bb82ed2 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Wed, 10 Sep 2025 18:17:39 +0000 Subject: [PATCH 12/32] Some early efforts on understanding problems with block_start improvements --- .../gc/shenandoah/shenandoahMarkBitMap.cpp | 14 +- .../gc/shenandoah/shenandoahMarkBitMap.hpp | 34 +- .../shenandoahMarkBitMap.inline.hpp | 7 + .../shenandoah/shenandoahMarkingContext.hpp | 4 +- .../shenandoahMarkingContext.inline.hpp | 2 +- .../shenandoah/shenandoahScanRemembered.cpp | 11 +- .../shenandoah/test_shenandoahMarkBitMap.cpp | 550 ++++++++++++++++++ 7 files changed, 610 insertions(+), 12 deletions(-) create mode 100644 test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp index 401ecdc1476e4..048081c71e8d0 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp @@ -57,6 +57,13 @@ bool ShenandoahMarkBitMap::is_bitmap_clear_range(const HeapWord* start, const He return (result == end); } +#ifdef KELVIN_THINKING_OUT_LOUD +// This is new functionality intorudced by this PR. I want to change +// the name to get_prev_marked_addr. I want to clarify that we search over the range [limit, addr) and +// that the Sentinel value addr denotes no previous marked object found. + +// This comment is present in the .hpp file. +#endif HeapWord* ShenandoahMarkBitMap::get_last_marked_addr(const HeapWord* limit, const HeapWord* addr) const { @@ -70,11 +77,12 @@ HeapWord* ShenandoahMarkBitMap::get_last_marked_addr(const HeapWord* limit, assert(addr <= tams, "addr must be less than TAMS"); #endif - // Round addr up to a possible object boundary to be safe. + // Round addr down to a possible object boundary to be safe. size_t const addr_offset = address_to_index(align_down(addr, HeapWordSize << LogMinObjAlignment)); size_t const limit_offset = address_to_index(limit); - size_t const nextOffset = get_last_one_offset(limit_offset, addr_offset); - return index_to_address(nextOffset); + size_t const last_offset = get_last_one_offset(limit_offset, addr_offset); + // cast required to remove const-ness of the value pointed to. We won't modify that object, but my caller might. + return (last_offset == addr_offset)? (HeapWord*) addr: index_to_address(last_offset); } HeapWord* ShenandoahMarkBitMap::get_next_marked_addr(const HeapWord* addr, diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp index faa7abb531090..f0c3cad958b33 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp @@ -112,6 +112,18 @@ class ShenandoahMarkBitMap { return raw_to_words_align_down(bit); } +#ifdef KELVIN_THINKING_OUT_LOUD + // kelvin wants to redefine the flip parameter in the following, after he understands the code + // better. the current comments are a bit cryptic. + + // naming should be get_first_matching_bit() and get_last_matching_bit() + // comments should clarify the ranges examined to find the bit, and what value is returned + // in case searched bit is not found + // rename flip template parameter as match_one_bit (false means match_zero bit) + + +#endif + // Helper for get_next_{zero,one}_bit variants. // - flip designates whether searching for 1s or 0s. Must be one of // find_{zeros,ones}_flip. @@ -126,8 +138,17 @@ class ShenandoahMarkBitMap { template inline idx_t get_last_bit_impl(idx_t l_index, idx_t r_index) const; +#ifdef KELVIN_THINKING_OUT_LOUD + // kelvin wants to rename get_next_one_offset() as get_first_one_offset + // would like search space to be specified as (l_index, r_index]. Return l_index if not found. + + // does this require changes to implementation and invocations? TBD +#endif + inline idx_t get_next_one_offset (idx_t l_index, idx_t r_index) const; + + // Search for last one in the range [l_index, r_index). Return r_index if not found. inline idx_t get_last_one_offset (idx_t l_index, idx_t r_index) const; void clear_large_range (idx_t beg, idx_t end); @@ -176,8 +197,17 @@ class ShenandoahMarkBitMap { HeapWord* get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const; - // Return the last marked address in the range (limit, addr], or limit - // if none found. +#ifdef KELVIN_THINKING_OUT_LOAD + // I haven't changed this code yet, but I'm thinking the following change would also be appropriate: + // get_next_marked_addr() should be renamed get_first_marked_addr + // The search domain for get_first_marked_addr should be (addr, limit) and not-found sentinel is limit + // (looking for first marked object that comes after addr) + + // Might have to adjust the invocations of these two functions, or maybe they already behave + // this way and the commentaries are wrong. +#endif + + // Return the last marked address in the range [limit, addr), or addr if none found. HeapWord* get_last_marked_addr(const HeapWord* limit, const HeapWord* addr) const; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp index 44c716c501312..71632cad4164d 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp @@ -171,6 +171,13 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_bit_impl(idx_t return r_index; } +#ifdef KELVIN_THINKING_OUT_LOUND +// Make cleaer that we find that last_bit in the range [l_index, r_index), returning r_index if none found + +// kelvin thinks the implementation is currently wrong, returning l_index for failure to find one. + +// kelvin also prefers to use all caps for template parameters. +#endif template inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_last_bit_impl(idx_t l_index, idx_t r_index) const { STATIC_ASSERT(flip == find_ones_flip || flip == find_zeros_flip); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp index 1710f34bbd77f..82a975d8330ff 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp @@ -69,8 +69,8 @@ class ShenandoahMarkingContext : public CHeapObj { // get the first marked address in the range [addr,linit), or limit if none found inline HeapWord* get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const; - // get the last marked address in the range (limit, addr), or limit if none found - inline HeapWord* get_last_marked_addr(const HeapWord* limit, const HeapWord* addr) const; + // get the last marked address in the range [limit, addr), or addr if none found + inline HeapWord* get_prev_marked_addr(const HeapWord* limit, const HeapWord* addr) const; inline bool allocated_after_mark_start(const oop obj) const; inline bool allocated_after_mark_start(const HeapWord* addr) const; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp index db7ee03046fb5..f3853a1936a1f 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp @@ -72,7 +72,7 @@ inline HeapWord* ShenandoahMarkingContext::get_next_marked_addr(const HeapWord* return _mark_bit_map.get_next_marked_addr(start, limit); } -inline HeapWord* ShenandoahMarkingContext::get_last_marked_addr(const HeapWord* limit, const HeapWord* start) const { +inline HeapWord* ShenandoahMarkingContext::get_prev_marked_addr(const HeapWord* limit, const HeapWord* start) const { return _mark_bit_map.get_last_marked_addr(limit, start); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 6fbc9c0e1abd1..1733e84762073 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -256,8 +256,8 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } // get the previous marked object, if any if (region->bottom() < left) { - HeapWord* prev = ctx->get_last_marked_addr(region->bottom(), left); - if (prev != nullptr) { + HeapWord* prev = ctx->get_prev_marked_addr(region->bottom(), left); + if (prev < left) { oop obj = cast_to_oop(prev); assert(oopDesc::is_oop(obj), "Should be an object"); HeapWord* obj_end = prev + obj->size(); @@ -266,16 +266,19 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } } } - // the prev marked object, if any, ends before left; - // find the next marked object if any on this card + // Either prev >= left (no previous object found), or the previous object that was found ends before my card range begins. + // In eiher case, find the next marked object if any on this card assert(!ctx->is_marked(left), "Was dealt with above"); HeapWord* right = MIN2(region->top(), ctx->top_at_mark_start(region)); assert(right > left, "We don't expect to be examining cards above the smaller of TAMS or top"); HeapWord* next = ctx->get_next_marked_addr(left, right); +#ifdef ASSERT if (next < right) { oop obj = cast_to_oop(next); assert(oopDesc::is_oop(obj), "Should be an object"); } +#endif + // Note: returned value may point beyond this card's range of memory, and may not point to an allocated object. return next; } diff --git a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp new file mode 100644 index 0000000000000..c5968fa5dd464 --- /dev/null +++ b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp @@ -0,0 +1,550 @@ +/* + * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +#include "memory/memRegion.hpp" +#include "gc/shenandoah/shenandoahMarkBitMap.hpp" +#include "gc/shenandoah/shenandoahMarkBitMap.inline.hpp" +#include "utilities/ostream.hpp" + +#include "utilities/vmassert_uninstall.hpp" +BEGIN_ALLOW_FORBIDDEN_FUNCTIONS +#include +END_ALLOW_FORBIDDEN_FUNCTIONS +#include "utilities/vmassert_reinstall.hpp" +#include "unittest.hpp" + +static bool _success; +static size_t _assertion_failures; + +#define MarkBitMapAssert(predicate) ASSERT_TRUE((predicate); if (!(predicate)) { _assertion_failures++; } +#define MarkBitMapAssert_EQ(a, b) ASSERT_EQ(((a), (b)); if ((a) != (b)) { _assertion_failures++; } + + +class ShenandoahMarkBitMapTest: public ::testing::Test { +protected: + + // 1 MB heap size + static const size_t HEAP_SIZE = 1024 * 1024; + static unsigned HeapWord my_heap_memory[HEAP_SIZE / HeapWordSize]; + static HeapWord* end_of_my_heap = &my_heap_memory[HEAP_SIZE / HeapWordSize]; + + + void verify_bitmap_is_empty(HeapWord *start, size_t words_in_heap, ShenandoahMarkBitMap* mbm) { + MarkBitMapAssert(mbm->is_bitmap_clear_range(start, start + words_in_heap)); + while (words_in_heap-- > 0) { + MarkBitMapAssert(!mbm->is_marked(start)); + MarkBitMapAssert(!mbm->is_marked_weak(start)); + MarkBitMapAssert(!mbm->is_marked_strong(start)); + start++; + } + } + + void verify_bitmap_is_weakly_marked(ShenandoahMarkBitMap* mbm, + HeapWord* strongly_marked_addresses[], size_t strongly_marked_objects) { + for (size_t i = 0; i < strong_marked_objects; i++) { + HeapWord* obj_addr = strongl_marked_address[i]; + MarkBitMapAssert(mbm->is_marked(obj_addr)); + MarkBitMapAssert(mbm->is_marked_weak(obj_addr)); + } + } + + void verify_bitmap_is_strongly_marked(ShenandoahMarkBitMap* mbm, + HeapWord* strongly_marked_addresses[], size_t strongly_marked_objects) { + for (size_t i = 0; i < strong_marked_objects; i++) { + HeapWord* obj_addr = strongl_marked_address[i]; + MarkBitMapAssert(mbm->is_marked(obj_addr)); + MarkBitMapAssert(mbm->is_marked_strong(obj_addr)); + } + } + + void verify_bitmap_all(ShenandoahMarkBitMap* mbm, HeapWord* all_marked_addresses[], + bool is_weakly_marked_object[], bool is_strongly_marked_object[], size_t all_marked_objects) { + HeapWord* last_marked_addr = &my_heap_memory[-1]; + for (size_t i = 0; i < all_marked_objects; i++) { + HeapWord* obj_addr = strongl_marked_address[i]; + if (is_strongly_marked_object[i]) { + MarkBitMapAssert(mbm->is_marked(obj_addr)); + MarkBitMapAssert(mbm->is_marked_strong(obj_addr)); + } + if (is_weakly_marked_object[i]) { + MarkBitMapAssert(mbm->is_marked(obj_addr)); + MarkBitMapAssert(mbm->is_marked_weak(obj_addr)); + } + while (++last_marked_addr < obj_addr) { + MarkBitMapAssert(!mbm-is_marked(last_marked_addr)); + MarkBitMapAssert(!mbm-is_marked_strong(last_marked_addr)); + MarkBitMapAssert(!mbm-is_marked_weak(last_marked_addr)); + } + last_marked_addr = obj_addr; + } + while (++last_marked_addr < end_of_my_heap) { + MarkBitMapAssert(!mbm-is_marked(last_marked_addr)); + MarkBitMapAssert(!mbm-is_marked_strong(last_marked_addr)); + MarkBitMapAssert(!mbm-is_marked_weak(last_marked_addr)); + } + + HeapWord* next_marked = (HeapWord*) &my_heap_memory[0]; + for (int i = 0; i < weakly_marked_objects; i++) { + next_marked = mbm.get_next_marked_addr(next_marked, end_of_my_heap); + MarkBitMapAssert(mbm->is_marked(next_marked)); + MarkBitMapAssert_EQ(next_marked, all_marked_addresses[i]); + if (is_strongly_marked_object[i]) { + MarkBitMapAssert(mbm->is_marked(obj_addr)); + MarkBitMapAssert(mbm->is_marked_strong(obj_addr)); + } + if (is_weakly_marked_object[i]) { + MarkBitMapAssert(mbm->is_marked(obj_addr)); + MarkBitMapAssert(mbm->is_marked_weak(obj_addr)); + } + } + // We expect no more marked addresses to be found. Should return limit. + HeapWord* sentinel = mbm->get_next_marked_addr(next_marked, end_of_my_heap); + MarkBitMapAssert_EQ(sentinel, end_of_my_heap); + + HeapWord* prev_marked = end_of_my_heap;; + for (int i = weakly_marked_objects - 1; i >= 0; i--) { + // to be renamed get_prev_marked_addr() + prev_marked = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked); + MarkBitMapAssert_EQ(prev_marked, all_marked_addresses[i]); + MarkBitMapAssert(mbm->is_marked(prev_marked)); + if (is_strongly_marked_object[i]) { + MarkBitMapAssert(mbm->is_marked(obj_addr)); + MarkBitMapAssert(mbm->is_marked_strong(obj_addr)); + } + if (is_weakly_marked_object[i]) { + MarkBitMapAssert(mbm->is_marked(obj_addr)); + MarkBitMapAssert(mbm->is_marked_weak(obj_addr)); + } + } + // We expect no more marked addresses to be found. should return prev_marked. + sentinel = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked); + MarkBitMapAssert_EQ(sentinel, prev_marked); + } + +public: + + static bool run_test() { + + _success = false; + _assertion_failures = 0; + + size_t bitmap_page_size = UseLargePages ? os::large_page_size() : os::vm_page_size(); + size_t bitmap_size_orig = ShenandoahMarkBiMap::compute_size(HEAP_SIZE); + size_t bitmap_size = align_up(bitmap_size_orig, bitmap_page_size); + + HeapWord* my_bitmap_memory = (HeapWord*) malloc(bitmap_size); + assert(my_bitmap_memory != nullptr, "Cannot run test"); + + size_t heap_size_words = HEAP_SIZE / HeapWordSize; + MemoryRegion heap_descriptor(my_heap, heap_size_words); + MemoryRegion bitmap_descriptor(my_bitmap_memory, bitmap_size / HeapWordSize); + + // Instantiate the mark bit map + ShenandoahMarkBitMap mbm(heap_descriptor, bitmap_descriptor); + + mbp->clear_large_range(heap_descriptor); + verify_bitmap_is_empty((HeapWord*) my_heap_memory, heap_size_words, &mbm); + + HeapWord* weakly_marked_addresses[] = { + (HeapWord*) &my_heap_memory[13], + (HeapWord*) &my_heap_memory[14], + (HeapWord*) &my_heap_memory[15], + (HeapWord*) &my_heap_memory[16], + (HeapWord*) &my_heap_memory[176], + (HeapWord*) &my_heap_memory[240], + (HeapWord*) &my_heap_memory[480], + (HeapWord*) &my_heap_memory[1360], + (HeapWord*) &my_heap_memory[1488], + (HeapWord*) &my_heap_memory[2416], + (HeapWord*) &my_heap_memory[5968], + + (HeapWord*) &my_heap_memory[8191], + (HeapWord*) &my_heap_memory[8192], + (HeapWord*) &my_heap_memory[8193] + }; + size_t weakly_marked_objects = sizeof(weakly_marked_addresses) / sizeof(HeapWord*); + + for (int i = 0; i < sizeof(weakly_marked_addresses) / sizeof(HeapWord *); i++) { + mbm->mark_weak(weakly_marked_addresses[i]); + } + + HeapWord* next_marked = (HeapWord*) &my_heap_memory[0]; + for (int i = 0; i < weakly_marked_objects; i++) { + next_marked = mbm.get_next_marked_addr(next_marked, end_of_my_heap); + MarkBitMapAssert_EQ(next_marked, weakly_marked_addresses[i]); + MarkBitMapAssert(mbm.is_marked(next_marked)); + MarkBitMapAssert(mbm.is_marked_weak(next_marked)); + MarkBitMapAssert(!mbm.is_marked_strong(next_marked)); + } + // We expect no more marked addresses to be found. Should return limit. + HeapWord* sentinel = mbm.get_next_marked_addr(next_marked, end_of_my_heap); + MarkBitMapAssert_EQ(sentinel, end_of_my_heap); + + HeapWord* prev_marked = end_of_my_heap;; + for (int i = weakly_marked_objects - 1; i >= 0; i--) { + // to be renamed get_prev_marked_addr() + prev_marked = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked); + MarkBitMapAssert_EQ(prev_marked, weakly_marked_addresses[i]); + MarkBitMapAssert(mbm.is_marked(prev_marked)); + MarkBitMapAssert(mbm.is_marked_weak(prev_marked)); + MarkBitMapAssert(!mbm.is_marked_strong(prev_marked)); + } + // We expect no more marked addresses to be found. should return prev_marked. + sentinel = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked); + MarkBitMapAssert_EQ(sentinel, prev_marked); + + verify_bitmap_is_weakly_marked(&mbm, weakly_marked_addresses, weakly_marked_objects); + + HeapWord* strongly_marked_addresses[] = { + (HeapWord*) &my_heap_memory[8], + (HeapWord*) &my_heap_memory[24], + (HeapWord*) &my_heap_memory[32], + (HeapWord*) &my_heap_memory[56], + (HeapWord*) &my_heap_memory[64], + (HeapWord*) &my_heap_memory[168], + (HeapWord*) &my_heap_memory[232], + (HeapWord*) &my_heap_memory[248], + (HeapWord*) &my_heap_memory[256], + (HeapWord*) &my_heap_memory[257], + (HeapWord*) &my_heap_memory[258], + (HeapWord*) &my_heap_memory[259], + (HeapWord*) &my_heap_memory[488], + (HeapWord*) &my_heap_memory[1352], + (HeapWord*) &my_heap_memory[1496], + (HeapWord*) &my_heap_memory[2432], + (HeapWord*) &my_heap_memory[5960] + }; + size_t strongly_marked_objects = sizeof(strongly_marked_addresses) / sizeof(HeapWord*); + for (int i = 0; i < strongly_marked_objects; i++) { + bool upgraded = false; + mbm->mark_strong(weakly_marked_addresses[i], upgraded); + MarkBitMapAssert(!upgraded); + } + verify_bitmap_is_strongly_marked(&mbm, strongly_marked_addresses, strongly_marked_objects); + + HeapWord* upgraded_weakly_marked_addresses[] = { + (HeapWord*) &my_heap_memory[240], + (HeapWord*) &my_heap_memory[1360], + }; + size_t upgraded_weakly_marked_objects = sizeof(upgraded_weakly_marked_addresses) / sizeof(HeapWord *); + for (int i = 0; i < upgraded_weakly_marked_objects; i++) { + bool upgraded = false; + mbm->mark_strong(weakly_marked_addresses[i], upgraded); + MarkBitMapAssert(upgraded); + } + verify_bitmap_is_strongly_marked(&mbm, upgraded_weakly_marked_addresses, upgraded_weakly_marked_objects); + + HeapWord* all_marked_addresses[] = { + (HeapWord*) &my_heap_memory[8], /* strongly marked */ + (HeapWord*) &my_heap_memory[13], /* weakly marked */ + (HeapWord*) &my_heap_memory[14], /* weakly marked */ + (HeapWord*) &my_heap_memory[15], /* weakly marked */ + (HeapWord*) &my_heap_memory[16], /* weakly marked */ + (HeapWord*) &my_heap_memory[24], /* strongly marked */ + (HeapWord*) &my_heap_memory[32], /* strongly marked */ + (HeapWord*) &my_heap_memory[56], /* strongly marked */ + (HeapWord*) &my_heap_memory[64], /* strongly marked */ + (HeapWord*) &my_heap_memory[168], /* strongly marked */ + (HeapWord*) &my_heap_memory[176], /* weakly marked */ + (HeapWord*) &my_heap_memory[232], /* strongly marked */ + (HeapWord*) &my_heap_memory[240], /* weakly marked upgraded to strongly marked */ + (HeapWord*) &my_heap_memory[248], /* strongly marked */ + (HeapWord*) &my_heap_memory[256], /* strongly marked */ + (HeapWord*) &my_heap_memory[257], /* strongly marked */ + (HeapWord*) &my_heap_memory[258], /* strongly marked */ + (HeapWord*) &my_heap_memory[259], /* strongly marked */ + (HeapWord*) &my_heap_memory[480], /* weakly marked */ + (HeapWord*) &my_heap_memory[488], /* strongly marked */ + (HeapWord*) &my_heap_memory[1352], /* strongly marked */ + (HeapWord*) &my_heap_memory[1360], /* weakly marked upgraded to strongly marked */ + (HeapWord*) &my_heap_memory[1488], /* weakly marked */ + (HeapWord*) &my_heap_memory[1496], /* strongly marked */ + (HeapWord*) &my_heap_memory[2416], /* weakly marked */ + (HeapWord*) &my_heap_memory[2432], /* strongly marked */ + (HeapWord*) &my_heap_memory[5960] /* strongly marked */ + (HeapWord*) &my_heap_memory[5968], /* weakly marked */ + (HeapWord*) &my_heap_memory[8191], /* weakly marked */ + (HeapWord*) &my_heap_memory[8192], /* weakly marked */ + (HeapWord*) &my_heap_memory[8193] /* weakly marked */ + }; + size_t all_marked_objects = sizeof(all_marked_addresses) / sizeof(HeapWord*); + bool is_weakly_marked_object[] = { + false, + true, + true, + true, + true, + false, + false, + false, + false, + false, + true, + false, + true, + false, + false, + false, + false, + false, + true, + false, + false, + true, + true, + false, + true, + false, + false, + true, + true, + true, + true + }; + bool is_strongly_marked_object[] = { + true, + false, + false, + false, + false, + true, + true, + true, + true, + true, + false, + true, + true, + true, + true, + true, + true, + true, + false, + true, + true, + true, + false, + true, + false, + true, + true, + false, + false, + false, + false + }; + verify_bitmap_all(&mbm, all_marked_addresses, is_weakly_marked_object, is_strongly_marked_object, all_marked_objects); + + + MemRegion first_clear_region(&my_heap_memory[168], &my_heap_memory[256]); + mbm.clear_range_large(first_clear_region); + // Five objects are no longer marked + + HeapWord* all_marked_addresses_after_first_clear[] = { + (HeapWord*) &my_heap_memory[8], /* strongly marked */ + (HeapWord*) &my_heap_memory[13], /* weakly marked */ + (HeapWord*) &my_heap_memory[14], /* weakly marked */ + (HeapWord*) &my_heap_memory[15], /* weakly marked */ + (HeapWord*) &my_heap_memory[16], /* weakly marked */ + (HeapWord*) &my_heap_memory[24], /* strongly marked */ + (HeapWord*) &my_heap_memory[32], /* strongly marked */ + (HeapWord*) &my_heap_memory[56], /* strongly marked */ + (HeapWord*) &my_heap_memory[64], /* strongly marked */ + (HeapWord*) &my_heap_memory[256], /* strongly marked */ + (HeapWord*) &my_heap_memory[257], /* strongly marked */ + (HeapWord*) &my_heap_memory[258], /* strongly marked */ + (HeapWord*) &my_heap_memory[259], /* strongly marked */ + (HeapWord*) &my_heap_memory[480], /* weakly marked */ + (HeapWord*) &my_heap_memory[488], /* strongly marked */ + (HeapWord*) &my_heap_memory[1352], /* strongly marked */ + (HeapWord*) &my_heap_memory[1360], /* weakly marked upgraded to strongly marked */ + (HeapWord*) &my_heap_memory[1488], /* weakly marked */ + (HeapWord*) &my_heap_memory[1496], /* strongly marked */ + (HeapWord*) &my_heap_memory[2416], /* weakly marked */ + (HeapWord*) &my_heap_memory[2432], /* strongly marked */ + (HeapWord*) &my_heap_memory[5960] /* strongly marked */ + (HeapWord*) &my_heap_memory[5968], /* weakly marked */ + (HeapWord*) &my_heap_memory[8191], /* weakly marked */ + (HeapWord*) &my_heap_memory[8192], /* weakly marked */ + (HeapWord*) &my_heap_memory[8193] /* weakly marked */ + }; + size_t all_marked_objects_after_first_clear = sizeof(all_marked_addresses_after_first_clear) / sizeof(HeapWord*); + bool is_weakly_marked_object_after_first_clear[] = { + false, + true, + true, + true, + true, + false, + false, + false, + false, + false, + false, + false, + false, + true, + false, + false, + true, + true, + false, + true, + false, + false, + true, + true, + true, + true + }; + bool is_strongly_marked_object_after_first_clear[] = { + true, + false, + false, + false, + false, + true, + true, + true, + true, + true, + true, + true, + true, + false, + true, + true, + true, + false, + true, + false, + true, + true, + false, + false, + false, + false + }; + verify_bitmap_all(&mbm, all_marked_addresses_after_first_clear, + is_weakly_marked_object_after_first_clear, is_strongly_marked_object_after_first_cler, + all_marked_objects_after_first_clear); + + + MemRegion second_clear_region(&my_heap_memory[2360], &my_heap_memory[2416]); + mbm.clear_range_large(second_clear_region); + // Five objects are no longer marked + + HeapWord* all_marked_addresses_after_2nd_clear[] = { + (HeapWord*) &my_heap_memory[8], /* strongly marked */ + (HeapWord*) &my_heap_memory[13], /* weakly marked */ + (HeapWord*) &my_heap_memory[14], /* weakly marked */ + (HeapWord*) &my_heap_memory[15], /* weakly marked */ + (HeapWord*) &my_heap_memory[16], /* weakly marked */ + (HeapWord*) &my_heap_memory[24], /* strongly marked */ + (HeapWord*) &my_heap_memory[32], /* strongly marked */ + (HeapWord*) &my_heap_memory[56], /* strongly marked */ + (HeapWord*) &my_heap_memory[64], /* strongly marked */ + (HeapWord*) &my_heap_memory[256], /* strongly marked */ + (HeapWord*) &my_heap_memory[257], /* strongly marked */ + (HeapWord*) &my_heap_memory[258], /* strongly marked */ + (HeapWord*) &my_heap_memory[259], /* strongly marked */ + (HeapWord*) &my_heap_memory[480], /* weakly marked */ + (HeapWord*) &my_heap_memory[488], /* strongly marked */ + (HeapWord*) &my_heap_memory[1352], /* strongly marked */ + (HeapWord*) &my_heap_memory[2416], /* weakly marked */ + (HeapWord*) &my_heap_memory[2432], /* strongly marked */ + (HeapWord*) &my_heap_memory[5960] /* strongly marked */ + (HeapWord*) &my_heap_memory[5968], /* weakly marked */ + (HeapWord*) &my_heap_memory[8191], /* weakly marked */ + (HeapWord*) &my_heap_memory[8192], /* weakly marked */ + (HeapWord*) &my_heap_memory[8193] /* weakly marked */ + }; + size_t all_marked_objects_after_2nd_clear = sizeof(all_marked_addresses_after_2nd_clear) / sizeof(HeapWord*); + bool is_weakly_marked_object_after_2nd_clear[] = { + false, + true; + true; + true; + true; + false, + false, + false, + false, + false, + false, + false, + false, + true, + false, + false, + true, + false, + false, + true, + true, + true, + true + }; + bool is_strongly_marked_object_after_2nd_clear[] = { + true, + false, + false, + false, + false, + true, + true, + true, + true, + true, + true, + true, + true, + false, + true, + true, + false, + true, + true, + false, + false, + false, + false + }; + verify_bitmap_all(&mbm, all_marked_addresses_after_2nd_clear, + is_weakly_marked_object_after_2nd_clear, is_strongly_marked_object_after_2nd_cler, + all_marked_objects_after_2nd_clear); + + _success = true; + return true; + } + +}; + +TEST(BasicShenandoahMarkBitMapTest, minimum_test) { + + bool result = ShenandoahMarkBitMapTest::run_test(); + ASSERT_EQ(result, true); + ASSERT_EQ(_success, true); + ASSERT_EQ(_assertion_failures, (size_t) 0); +} From 7b48ebf2dbbcf057af2797e6178d48dab2d3f38f Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Mon, 15 Sep 2025 23:02:53 +0000 Subject: [PATCH 13/32] some progress on get_last_marked_object() --- .../gc/shenandoah/shenandoahMarkBitMap.cpp | 10 +- .../gc/shenandoah/shenandoahMarkBitMap.hpp | 2 +- .../shenandoahMarkBitMap.inline.hpp | 61 ++-- .../shenandoah/shenandoahMarkingContext.hpp | 5 +- .../shenandoah/shenandoahScanRemembered.cpp | 2 +- .../shenandoah/test_shenandoahMarkBitMap.cpp | 267 ++++++++++-------- .../test_shenandoahOldHeuristic.cpp | 2 +- 7 files changed, 197 insertions(+), 152 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp index 048081c71e8d0..9cf553e9ba8ac 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp @@ -58,10 +58,9 @@ bool ShenandoahMarkBitMap::is_bitmap_clear_range(const HeapWord* start, const He } #ifdef KELVIN_THINKING_OUT_LOUD -// This is new functionality intorudced by this PR. I want to change -// the name to get_prev_marked_addr. I want to clarify that we search over the range [limit, addr) and -// that the Sentinel value addr denotes no previous marked object found. - +// This is new functionality introduced by this PR. I want to change +// the name to get_prev_marked_addr. I want to clarify that we search over the range [limit, addr] and +// that the Sentinel value addr+1 denotes no previous marked object found. // This comment is present in the .hpp file. #endif @@ -81,8 +80,9 @@ HeapWord* ShenandoahMarkBitMap::get_last_marked_addr(const HeapWord* limit, size_t const addr_offset = address_to_index(align_down(addr, HeapWordSize << LogMinObjAlignment)); size_t const limit_offset = address_to_index(limit); size_t const last_offset = get_last_one_offset(limit_offset, addr_offset); + // cast required to remove const-ness of the value pointed to. We won't modify that object, but my caller might. - return (last_offset == addr_offset)? (HeapWord*) addr: index_to_address(last_offset); + return (last_offset > addr_offset)? (HeapWord*) addr + 1: index_to_address(last_offset); } HeapWord* ShenandoahMarkBitMap::get_next_marked_addr(const HeapWord* addr, diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp index f0c3cad958b33..2e1172affdb5e 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp @@ -207,7 +207,7 @@ class ShenandoahMarkBitMap { // this way and the commentaries are wrong. #endif - // Return the last marked address in the range [limit, addr), or addr if none found. + // Return the last marked address in the range [limit, addr], or addr+1 if none found. HeapWord* get_last_marked_addr(const HeapWord* limit, const HeapWord* addr) const; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp index 71632cad4164d..ba138e8004792 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp @@ -30,6 +30,7 @@ #include "runtime/atomic.hpp" #include "utilities/count_trailing_zeros.hpp" +#include "utilities/count_leading_zeros.hpp" inline size_t ShenandoahMarkBitMap::address_to_index(const HeapWord* addr) const { return (pointer_delta(addr, _covered.start()) << 1) >> _shift; @@ -135,6 +136,9 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_bit_impl(idx_t // Get the word containing l_index, and shift out low bits. idx_t index = to_words_align_down(l_index); bm_word_t cword = (map(index) ^ flip) >> bit_in_word(l_index); +#ifdef KELVIN_THINKING_OUT_LOUD + // the following test should look at (cword & 0x3) +#endif if ((cword & 1) != 0) { // The first bit is similarly often interesting. When it matters // (density or features of the calling algorithm make it likely @@ -202,43 +206,66 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_last_bit_impl(idx_t // an interesting bit will be present. if (l_index < r_index) { - // Get the word containing r_index, and shift out low bits. + // Get the word containing r_index, and shift out the high-order bits (representing objects that come after r_index) idx_t index = to_words_align_down(r_index); - bm_word_t cword = (map(index) ^ flip) >> bit_in_word(r_index); - if ((cword & 1) != 0) { - // The first bit is similarly often interesting. When it matters + assert(BitsPerWord - 2 >= bit_in_word(r_index), "sanity"); + size_t shift = BitsPerWord - 2 - bit_in_word(r_index); + bm_word_t cword = (map(index) ^ flip) << shift; + // After this shift, the highest order bits correspond to r_index. + + // With 64-bit words, + // We give special handling if either of the two most significant bits (Weak or Strong) is set. With 64-bit + // words, the mask of interest is 0xc000_0000_0000_0000. Symbolically, this constant is represented by: + const bm_word_t first_object_mask = ((bm_word_t) 0x3) << (BitsPerWord - 2); + if ((cword & first_object_mask) != 0) { + // The first object is similarly often interesting. When it matters // (density or features of the calling algorithm make it likely // the first bit is set), going straight to the next clause compares - // poorly with doing this check first; count_trailing_zeros can be + // poorly with doing this check first; count_leading_zeros can be // relatively expensive, plus there is the additional range check. // But when the first bit isn't set, the cost of having tested for // it is relatively small compared to the rest of the search. return r_index; } else if (cword != 0) { - // Flipped and shifted first word is non-zero. - idx_t result = r_index + count_trailing_zeros(cword); - if (aligned_left || (result > l_index)) return result; - // Result is beyond range bound; return l_index. + // Note that there are 2 bits corresponding to every index value (Weak and Strong), and every odd index value + // corresponds to the same object as index-1 + // Flipped and shifted first word is non-zero. If leading_zeros is 0 or 1, we return r_index (above). + // if leading zeros is 2 or 3, we return (r_index - 1) or (r_index - 2), and so forth + idx_t result = r_index + 1 - count_leading_zeros(cword); + if (aligned_left || (result >= l_index)) return result; + else { + // Sentinel value means no object found within specified range. + return r_index + 2; + } } else { // Flipped and shifted first word is zero. Word search through // aligned up r_index for a non-zero flipped word. idx_t limit = aligned_left ? to_words_align_down(l_index) // Minuscule savings when aligned. : to_words_align_up(l_index); - while (--index > limit) { + // Unsigned index is always >= unsigned limit if limit equals zero, so test for strictly greater than before decrement. + while (index-- > limit) { cword = map(index) ^ flip; if (cword != 0) { - idx_t result = bit_index(index) + count_trailing_zeros(cword); - if (aligned_left || (result > l_index)) return result; - // Result is beyond range bound; return r_index. - assert((index - 1) == limit, "invariant"); - break; + // cword hods bits: + // 0x03 for the object corresponding to index (and index+1) (count_leading_zeros is 62 or 63) + // 0x0c for the object corresponding to index + 2 (and index+3) (count_leading_zeros is 60 or 61) + // and so on. + idx_t result = bit_index(index + 1) - (count_leading_zeros(cword) + 1); + if (aligned_left || (result >= l_index)) return result; + else { + // Sentinel value means no object found within specified range. + return r_index + 2; + } } } - // No bits in range; return l_index. + // No bits in range; return r_index+2. + return r_index + 2; } } - return l_index; + else { + return r_index + 2; + } } inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_one_offset(idx_t l_offset, idx_t r_offset) const { diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp index 82a975d8330ff..a5ad5008b6a6f 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp @@ -67,9 +67,10 @@ class ShenandoahMarkingContext : public CHeapObj { inline bool is_marked_or_old(oop obj) const; inline bool is_marked_strong_or_old(oop obj) const; - // get the first marked address in the range [addr,linit), or limit if none found + // Return address of the first marked address in the range [addr,limit), or limit if no marked object found inline HeapWord* get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const; - // get the last marked address in the range [limit, addr), or addr if none found + + // Return address of the last marked object in range [limit, start], returning start+1 if no marked object found inline HeapWord* get_prev_marked_addr(const HeapWord* limit, const HeapWord* addr) const; inline bool allocated_after_mark_start(const oop obj) const; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 1733e84762073..43951b9bb9eb6 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -257,7 +257,7 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // get the previous marked object, if any if (region->bottom() < left) { HeapWord* prev = ctx->get_prev_marked_addr(region->bottom(), left); - if (prev < left) { + if (prev <= left) { oop obj = cast_to_oop(prev); assert(oopDesc::is_oop(obj), "Should be an object"); HeapWord* obj_end = prev + obj->size(); diff --git a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp index c5968fa5dd464..26727d3c4f900 100644 --- a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp +++ b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp @@ -21,7 +21,10 @@ * questions. */ +#include "unittest.hpp" + #include "memory/memRegion.hpp" +#include "gc/shenandoah/shenandoahHeap.inline.hpp" #include "gc/shenandoah/shenandoahMarkBitMap.hpp" #include "gc/shenandoah/shenandoahMarkBitMap.inline.hpp" #include "utilities/ostream.hpp" @@ -31,138 +34,158 @@ BEGIN_ALLOW_FORBIDDEN_FUNCTIONS #include END_ALLOW_FORBIDDEN_FUNCTIONS #include "utilities/vmassert_reinstall.hpp" -#include "unittest.hpp" + +// These tests will all be skipped (unless Shenandoah becomes the default +// collector). To execute these tests, you must enable Shenandoah, which +// is done with: +// +// % make exploded-test TEST="gtest:ShenandoahOld*" CONF=release TEST_OPTS="JAVA_OPTIONS=-XX:+UseShenandoahGC -XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational" +// +// Please note that these 'unit' tests are really integration tests and rely +// on the JVM being initialized. These tests manipulate the state of the +// collector in ways that are not compatible with a normal collection run. +// If these tests take longer than the minimum time between gc intervals - +// or, more likely, if you have them paused in a debugger longer than this +// interval - you can expect trouble. These tests will also not run in a build +// with asserts enabled because they use APIs that expect to run on a safepoint. + +// I may have to disable tests if this is a debug build, but let's try +// without that constraint first +// +#define SKIP_IF_NOT_SHENANDOAH() \ + if (!UseShenandoahGC) { \ + std::cout << "skipped\n"; \ + return; \ + } static bool _success; static size_t _assertion_failures; -#define MarkBitMapAssert(predicate) ASSERT_TRUE((predicate); if (!(predicate)) { _assertion_failures++; } -#define MarkBitMapAssert_EQ(a, b) ASSERT_EQ(((a), (b)); if ((a) != (b)) { _assertion_failures++; } +#define MarkBitMapAssertEqual(a, b) EXPECT_EQ((a), (b)); if ((a) != (b)) { _assertion_failures++; } +#define MarkBitMapAssertTrue(a) EXPECT_TRUE((a)); if ((a) == 0) { _assertion_failures++; } class ShenandoahMarkBitMapTest: public ::testing::Test { protected: - // 1 MB heap size - static const size_t HEAP_SIZE = 1024 * 1024; - static unsigned HeapWord my_heap_memory[HEAP_SIZE / HeapWordSize]; - static HeapWord* end_of_my_heap = &my_heap_memory[HEAP_SIZE / HeapWordSize]; - - - void verify_bitmap_is_empty(HeapWord *start, size_t words_in_heap, ShenandoahMarkBitMap* mbm) { - MarkBitMapAssert(mbm->is_bitmap_clear_range(start, start + words_in_heap)); + static void verify_bitmap_is_empty(HeapWord *start, size_t words_in_heap, ShenandoahMarkBitMap* mbm) { + MarkBitMapAssertTrue(mbm->is_bitmap_clear_range(start, start + words_in_heap)); while (words_in_heap-- > 0) { - MarkBitMapAssert(!mbm->is_marked(start)); - MarkBitMapAssert(!mbm->is_marked_weak(start)); - MarkBitMapAssert(!mbm->is_marked_strong(start)); + MarkBitMapAssertTrue(!mbm->is_marked(start)); + MarkBitMapAssertTrue(!mbm->is_marked_weak(start)); + MarkBitMapAssertTrue(!mbm->is_marked_strong(start)); start++; } } - void verify_bitmap_is_weakly_marked(ShenandoahMarkBitMap* mbm, - HeapWord* strongly_marked_addresses[], size_t strongly_marked_objects) { - for (size_t i = 0; i < strong_marked_objects; i++) { - HeapWord* obj_addr = strongl_marked_address[i]; - MarkBitMapAssert(mbm->is_marked(obj_addr)); - MarkBitMapAssert(mbm->is_marked_weak(obj_addr)); + static void verify_bitmap_is_weakly_marked(ShenandoahMarkBitMap* mbm, + HeapWord* weakly_marked_addresses[], size_t weakly_marked_objects) { + for (size_t i = 0; i < weakly_marked_objects; i++) { + HeapWord* obj_addr = weakly_marked_addresses[i]; + MarkBitMapAssertTrue(mbm->is_marked(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked_weak(obj_addr)); } } - void verify_bitmap_is_strongly_marked(ShenandoahMarkBitMap* mbm, - HeapWord* strongly_marked_addresses[], size_t strongly_marked_objects) { - for (size_t i = 0; i < strong_marked_objects; i++) { - HeapWord* obj_addr = strongl_marked_address[i]; - MarkBitMapAssert(mbm->is_marked(obj_addr)); - MarkBitMapAssert(mbm->is_marked_strong(obj_addr)); + static void verify_bitmap_is_strongly_marked(ShenandoahMarkBitMap* mbm, + HeapWord* strongly_marked_addresses[], size_t strongly_marked_objects) { + for (size_t i = 0; i < strongly_marked_objects; i++) { + HeapWord* obj_addr = strongly_marked_addresses[i]; + MarkBitMapAssertTrue(mbm->is_marked(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked_strong(obj_addr)); } } - void verify_bitmap_all(ShenandoahMarkBitMap* mbm, HeapWord* all_marked_addresses[], - bool is_weakly_marked_object[], bool is_strongly_marked_object[], size_t all_marked_objects) { - HeapWord* last_marked_addr = &my_heap_memory[-1]; + static void verify_bitmap_all(ShenandoahMarkBitMap* mbm, HeapWord* all_marked_addresses[], + bool is_weakly_marked_object[], bool is_strongly_marked_object[], size_t all_marked_objects, + HeapWord* heap_memory, HeapWord* end_of_heap_memory) { + HeapWord* last_marked_addr = &heap_memory[-1]; for (size_t i = 0; i < all_marked_objects; i++) { - HeapWord* obj_addr = strongl_marked_address[i]; + HeapWord* obj_addr = all_marked_addresses[i]; if (is_strongly_marked_object[i]) { - MarkBitMapAssert(mbm->is_marked(obj_addr)); - MarkBitMapAssert(mbm->is_marked_strong(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked_strong(obj_addr)); } if (is_weakly_marked_object[i]) { - MarkBitMapAssert(mbm->is_marked(obj_addr)); - MarkBitMapAssert(mbm->is_marked_weak(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked_weak(obj_addr)); } while (++last_marked_addr < obj_addr) { - MarkBitMapAssert(!mbm-is_marked(last_marked_addr)); - MarkBitMapAssert(!mbm-is_marked_strong(last_marked_addr)); - MarkBitMapAssert(!mbm-is_marked_weak(last_marked_addr)); + MarkBitMapAssertTrue(!mbm->is_marked(last_marked_addr)); + MarkBitMapAssertTrue(!mbm->is_marked_strong(last_marked_addr)); + MarkBitMapAssertTrue(!mbm->is_marked_weak(last_marked_addr)); } last_marked_addr = obj_addr; } - while (++last_marked_addr < end_of_my_heap) { - MarkBitMapAssert(!mbm-is_marked(last_marked_addr)); - MarkBitMapAssert(!mbm-is_marked_strong(last_marked_addr)); - MarkBitMapAssert(!mbm-is_marked_weak(last_marked_addr)); + while (++last_marked_addr < end_of_heap_memory) { + MarkBitMapAssertTrue(!mbm->is_marked(last_marked_addr)); + MarkBitMapAssertTrue(!mbm->is_marked_strong(last_marked_addr)); + MarkBitMapAssertTrue(!mbm->is_marked_weak(last_marked_addr)); } - HeapWord* next_marked = (HeapWord*) &my_heap_memory[0]; - for (int i = 0; i < weakly_marked_objects; i++) { - next_marked = mbm.get_next_marked_addr(next_marked, end_of_my_heap); - MarkBitMapAssert(mbm->is_marked(next_marked)); - MarkBitMapAssert_EQ(next_marked, all_marked_addresses[i]); + HeapWord* next_marked = (HeapWord*) &heap_memory[0] - 1; + for (size_t i = 0; i < all_marked_objects; i++) { + next_marked = mbm->get_next_marked_addr(next_marked + 1, end_of_heap_memory); + MarkBitMapAssertTrue(mbm->is_marked(next_marked)); + MarkBitMapAssertEqual(next_marked, all_marked_addresses[i]); if (is_strongly_marked_object[i]) { - MarkBitMapAssert(mbm->is_marked(obj_addr)); - MarkBitMapAssert(mbm->is_marked_strong(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked_strong(next_marked)); } if (is_weakly_marked_object[i]) { - MarkBitMapAssert(mbm->is_marked(obj_addr)); - MarkBitMapAssert(mbm->is_marked_weak(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked_weak(next_marked)); } } // We expect no more marked addresses to be found. Should return limit. - HeapWord* sentinel = mbm->get_next_marked_addr(next_marked, end_of_my_heap); - MarkBitMapAssert_EQ(sentinel, end_of_my_heap); + HeapWord* sentinel = mbm->get_next_marked_addr(next_marked + 1, end_of_heap_memory); + MarkBitMapAssertEqual(sentinel, end_of_heap_memory); - HeapWord* prev_marked = end_of_my_heap;; - for (int i = weakly_marked_objects - 1; i >= 0; i--) { + HeapWord* prev_marked = end_of_heap_memory + 1; + for (int i = (int) all_marked_objects - 1; i >= 0; i--) { // to be renamed get_prev_marked_addr() - prev_marked = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked); - MarkBitMapAssert_EQ(prev_marked, all_marked_addresses[i]); - MarkBitMapAssert(mbm->is_marked(prev_marked)); + prev_marked = mbm->get_last_marked_addr(&heap_memory[0], prev_marked - 1); + MarkBitMapAssertEqual(prev_marked, all_marked_addresses[i]); + MarkBitMapAssertTrue(mbm->is_marked(prev_marked)); if (is_strongly_marked_object[i]) { - MarkBitMapAssert(mbm->is_marked(obj_addr)); - MarkBitMapAssert(mbm->is_marked_strong(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked_strong(prev_marked)); } if (is_weakly_marked_object[i]) { - MarkBitMapAssert(mbm->is_marked(obj_addr)); - MarkBitMapAssert(mbm->is_marked_weak(obj_addr)); + MarkBitMapAssertTrue(mbm->is_marked_weak(prev_marked)); } } // We expect no more marked addresses to be found. should return prev_marked. - sentinel = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked); - MarkBitMapAssert_EQ(sentinel, prev_marked); + sentinel = mbm->get_last_marked_addr(&heap_memory[0], prev_marked - 1); + MarkBitMapAssertEqual(sentinel, prev_marked); } public: static bool run_test() { + ShenandoahHeap* heap = ShenandoahHeap::heap(); + size_t heap_size = heap->max_capacity(); + size_t heap_size_words = heap_size / HeapWordSize; + HeapWord* my_heap_memory = heap->base(); + HeapWord* end_of_my_heap = my_heap_memory + heap_size_words; + MemRegion heap_descriptor(my_heap_memory, heap_size_words); _success = false; _assertion_failures = 0; size_t bitmap_page_size = UseLargePages ? os::large_page_size() : os::vm_page_size(); - size_t bitmap_size_orig = ShenandoahMarkBiMap::compute_size(HEAP_SIZE); + size_t bitmap_size_orig = ShenandoahMarkBitMap::compute_size(heap_size); size_t bitmap_size = align_up(bitmap_size_orig, bitmap_page_size); + size_t bitmap_word_size = (bitmap_size + HeapWordSize - 1) / HeapWordSize; - HeapWord* my_bitmap_memory = (HeapWord*) malloc(bitmap_size); - assert(my_bitmap_memory != nullptr, "Cannot run test"); - - size_t heap_size_words = HEAP_SIZE / HeapWordSize; - MemoryRegion heap_descriptor(my_heap, heap_size_words); - MemoryRegion bitmap_descriptor(my_bitmap_memory, bitmap_size / HeapWordSize); + HeapWord* my_bitmap_memory = NEW_C_HEAP_ARRAY(HeapWord, bitmap_word_size, mtGC); - // Instantiate the mark bit map + MarkBitMapAssertTrue(my_bitmap_memory != nullptr); + if (my_bitmap_memory == nullptr) { + std::cout <<"Cannot run test because failed to allocate bitmap memory\n" << std::flush; + return false; + } + MemRegion bitmap_descriptor(my_bitmap_memory, bitmap_size / HeapWordSize); ShenandoahMarkBitMap mbm(heap_descriptor, bitmap_descriptor); - mbp->clear_large_range(heap_descriptor); + mbm.clear_range_large(heap_descriptor); verify_bitmap_is_empty((HeapWord*) my_heap_memory, heap_size_words, &mbm); HeapWord* weakly_marked_addresses[] = { @@ -177,42 +200,39 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { (HeapWord*) &my_heap_memory[1488], (HeapWord*) &my_heap_memory[2416], (HeapWord*) &my_heap_memory[5968], - (HeapWord*) &my_heap_memory[8191], (HeapWord*) &my_heap_memory[8192], (HeapWord*) &my_heap_memory[8193] }; size_t weakly_marked_objects = sizeof(weakly_marked_addresses) / sizeof(HeapWord*); - - for (int i = 0; i < sizeof(weakly_marked_addresses) / sizeof(HeapWord *); i++) { - mbm->mark_weak(weakly_marked_addresses[i]); + for (size_t i = 0; i < weakly_marked_objects; i++) { + mbm.mark_weak(weakly_marked_addresses[i]); } - - HeapWord* next_marked = (HeapWord*) &my_heap_memory[0]; - for (int i = 0; i < weakly_marked_objects; i++) { - next_marked = mbm.get_next_marked_addr(next_marked, end_of_my_heap); - MarkBitMapAssert_EQ(next_marked, weakly_marked_addresses[i]); - MarkBitMapAssert(mbm.is_marked(next_marked)); - MarkBitMapAssert(mbm.is_marked_weak(next_marked)); - MarkBitMapAssert(!mbm.is_marked_strong(next_marked)); + HeapWord* next_marked = (HeapWord*) &my_heap_memory[0] - 1; + for (size_t i = 0; i < weakly_marked_objects; i++) { + next_marked = mbm.get_next_marked_addr(next_marked + 1, end_of_my_heap); + MarkBitMapAssertEqual(next_marked, weakly_marked_addresses[i]); + MarkBitMapAssertTrue(mbm.is_marked(next_marked)); + MarkBitMapAssertTrue(mbm.is_marked_weak(next_marked)); + MarkBitMapAssertTrue(!mbm.is_marked_strong(next_marked)); } // We expect no more marked addresses to be found. Should return limit. - HeapWord* sentinel = mbm.get_next_marked_addr(next_marked, end_of_my_heap); - MarkBitMapAssert_EQ(sentinel, end_of_my_heap); - - HeapWord* prev_marked = end_of_my_heap;; + HeapWord* sentinel = mbm.get_next_marked_addr(next_marked + 1, end_of_my_heap); + HeapWord* heap_limit = end_of_my_heap; + MarkBitMapAssertEqual(sentinel, heap_limit); + HeapWord* prev_marked = end_of_my_heap + 1;; for (int i = weakly_marked_objects - 1; i >= 0; i--) { // to be renamed get_prev_marked_addr() - prev_marked = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked); - MarkBitMapAssert_EQ(prev_marked, weakly_marked_addresses[i]); - MarkBitMapAssert(mbm.is_marked(prev_marked)); - MarkBitMapAssert(mbm.is_marked_weak(prev_marked)); - MarkBitMapAssert(!mbm.is_marked_strong(prev_marked)); + prev_marked = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked - 1); + MarkBitMapAssertEqual(prev_marked, weakly_marked_addresses[i]); + MarkBitMapAssertTrue(mbm.is_marked(prev_marked)); + MarkBitMapAssertTrue(mbm.is_marked_weak(prev_marked)); + MarkBitMapAssertTrue(!mbm.is_marked_strong(prev_marked)); } // We expect no more marked addresses to be found. should return prev_marked. - sentinel = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked); - MarkBitMapAssert_EQ(sentinel, prev_marked); - + sentinel = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked - 1); + // MarkBitMapAssertEqual(sentinel, prev_marked); + MarkBitMapAssertEqual(sentinel, prev_marked); verify_bitmap_is_weakly_marked(&mbm, weakly_marked_addresses, weakly_marked_objects); HeapWord* strongly_marked_addresses[] = { @@ -235,22 +255,21 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { (HeapWord*) &my_heap_memory[5960] }; size_t strongly_marked_objects = sizeof(strongly_marked_addresses) / sizeof(HeapWord*); - for (int i = 0; i < strongly_marked_objects; i++) { + for (size_t i = 0; i < strongly_marked_objects; i++) { bool upgraded = false; - mbm->mark_strong(weakly_marked_addresses[i], upgraded); - MarkBitMapAssert(!upgraded); + mbm.mark_strong(strongly_marked_addresses[i], upgraded); + MarkBitMapAssertTrue(!upgraded); } verify_bitmap_is_strongly_marked(&mbm, strongly_marked_addresses, strongly_marked_objects); - HeapWord* upgraded_weakly_marked_addresses[] = { (HeapWord*) &my_heap_memory[240], (HeapWord*) &my_heap_memory[1360], }; size_t upgraded_weakly_marked_objects = sizeof(upgraded_weakly_marked_addresses) / sizeof(HeapWord *); - for (int i = 0; i < upgraded_weakly_marked_objects; i++) { + for (size_t i = 0; i < upgraded_weakly_marked_objects; i++) { bool upgraded = false; - mbm->mark_strong(weakly_marked_addresses[i], upgraded); - MarkBitMapAssert(upgraded); + mbm.mark_strong(upgraded_weakly_marked_addresses[i], upgraded); + MarkBitMapAssertTrue(upgraded); } verify_bitmap_is_strongly_marked(&mbm, upgraded_weakly_marked_addresses, upgraded_weakly_marked_objects); @@ -281,11 +300,11 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { (HeapWord*) &my_heap_memory[1496], /* strongly marked */ (HeapWord*) &my_heap_memory[2416], /* weakly marked */ (HeapWord*) &my_heap_memory[2432], /* strongly marked */ - (HeapWord*) &my_heap_memory[5960] /* strongly marked */ + (HeapWord*) &my_heap_memory[5960], /* strongly marked */ (HeapWord*) &my_heap_memory[5968], /* weakly marked */ - (HeapWord*) &my_heap_memory[8191], /* weakly marked */ - (HeapWord*) &my_heap_memory[8192], /* weakly marked */ - (HeapWord*) &my_heap_memory[8193] /* weakly marked */ + (HeapWord*) &my_heap_memory[8191], /* weakly marked */ + (HeapWord*) &my_heap_memory[8192], /* weakly marked */ + (HeapWord*) &my_heap_memory[8193] /* weakly marked */ }; size_t all_marked_objects = sizeof(all_marked_addresses) / sizeof(HeapWord*); bool is_weakly_marked_object[] = { @@ -354,13 +373,12 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { false, false }; - verify_bitmap_all(&mbm, all_marked_addresses, is_weakly_marked_object, is_strongly_marked_object, all_marked_objects); - + verify_bitmap_all(&mbm, all_marked_addresses, is_weakly_marked_object, is_strongly_marked_object, all_marked_objects, + my_heap_memory, end_of_my_heap); MemRegion first_clear_region(&my_heap_memory[168], &my_heap_memory[256]); mbm.clear_range_large(first_clear_region); // Five objects are no longer marked - HeapWord* all_marked_addresses_after_first_clear[] = { (HeapWord*) &my_heap_memory[8], /* strongly marked */ (HeapWord*) &my_heap_memory[13], /* weakly marked */ @@ -383,7 +401,7 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { (HeapWord*) &my_heap_memory[1496], /* strongly marked */ (HeapWord*) &my_heap_memory[2416], /* weakly marked */ (HeapWord*) &my_heap_memory[2432], /* strongly marked */ - (HeapWord*) &my_heap_memory[5960] /* strongly marked */ + (HeapWord*) &my_heap_memory[5960], /* strongly marked */ (HeapWord*) &my_heap_memory[5968], /* weakly marked */ (HeapWord*) &my_heap_memory[8191], /* weakly marked */ (HeapWord*) &my_heap_memory[8192], /* weakly marked */ @@ -447,14 +465,12 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { false }; verify_bitmap_all(&mbm, all_marked_addresses_after_first_clear, - is_weakly_marked_object_after_first_clear, is_strongly_marked_object_after_first_cler, - all_marked_objects_after_first_clear); + is_weakly_marked_object_after_first_clear, is_strongly_marked_object_after_first_clear, + all_marked_objects_after_first_clear, my_heap_memory, end_of_my_heap); - - MemRegion second_clear_region(&my_heap_memory[2360], &my_heap_memory[2416]); + MemRegion second_clear_region(&my_heap_memory[1360], &my_heap_memory[2416]); mbm.clear_range_large(second_clear_region); // Five objects are no longer marked - HeapWord* all_marked_addresses_after_2nd_clear[] = { (HeapWord*) &my_heap_memory[8], /* strongly marked */ (HeapWord*) &my_heap_memory[13], /* weakly marked */ @@ -474,7 +490,7 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { (HeapWord*) &my_heap_memory[1352], /* strongly marked */ (HeapWord*) &my_heap_memory[2416], /* weakly marked */ (HeapWord*) &my_heap_memory[2432], /* strongly marked */ - (HeapWord*) &my_heap_memory[5960] /* strongly marked */ + (HeapWord*) &my_heap_memory[5960], /* strongly marked */ (HeapWord*) &my_heap_memory[5968], /* weakly marked */ (HeapWord*) &my_heap_memory[8191], /* weakly marked */ (HeapWord*) &my_heap_memory[8192], /* weakly marked */ @@ -483,10 +499,10 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { size_t all_marked_objects_after_2nd_clear = sizeof(all_marked_addresses_after_2nd_clear) / sizeof(HeapWord*); bool is_weakly_marked_object_after_2nd_clear[] = { false, - true; - true; - true; - true; + true, + true, + true, + true, false, false, false, @@ -532,16 +548,17 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { false }; verify_bitmap_all(&mbm, all_marked_addresses_after_2nd_clear, - is_weakly_marked_object_after_2nd_clear, is_strongly_marked_object_after_2nd_cler, - all_marked_objects_after_2nd_clear); + is_weakly_marked_object_after_2nd_clear, is_strongly_marked_object_after_2nd_clear, + all_marked_objects_after_2nd_clear, my_heap_memory, end_of_my_heap); + FREE_C_HEAP_ARRAY(HeapWord, my_bitmap_memory); _success = true; return true; } - }; -TEST(BasicShenandoahMarkBitMapTest, minimum_test) { +TEST_VM_F(ShenandoahMarkBitMapTest, minimum_test) { + SKIP_IF_NOT_SHENANDOAH(); bool result = ShenandoahMarkBitMapTest::run_test(); ASSERT_EQ(result, true); diff --git a/test/hotspot/gtest/gc/shenandoah/test_shenandoahOldHeuristic.cpp b/test/hotspot/gtest/gc/shenandoah/test_shenandoahOldHeuristic.cpp index d08cfd7618bb0..7b633338c1f7a 100644 --- a/test/hotspot/gtest/gc/shenandoah/test_shenandoahOldHeuristic.cpp +++ b/test/hotspot/gtest/gc/shenandoah/test_shenandoahOldHeuristic.cpp @@ -49,7 +49,7 @@ #else #define SKIP_IF_NOT_SHENANDOAH() \ if (!UseShenandoahGC) { \ - tty->print_cr("skipped"); \ + std::cout << "skipped\n"; \ return; \ } #endif From ca728de194fab398a22aef5b57e78a20b8f59f3d Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Tue, 16 Sep 2025 16:18:47 -0600 Subject: [PATCH 14/32] Add special handling above TAMS --- .../shenandoah/shenandoahScanRemembered.cpp | 42 +++++++++++++++---- .../shenandoah/shenandoahScanRemembered.hpp | 3 +- .../shenandoahScanRemembered.inline.hpp | 11 ++++- 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 43951b9bb9eb6..7139d560fef54 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -232,7 +232,8 @@ size_t ShenandoahCardCluster::get_last_start(size_t card_index) const { // 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). -HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, const ShenandoahMarkingContext* const ctx) const { +HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, const ShenandoahMarkingContext* const ctx, + HeapWord* tams, const size_t last_relevant_card_index) const { HeapWord* left = _rs->addr_for_card_index(card_index); ShenandoahHeapRegion* region = ShenandoahHeap::heap()->heap_region_containing(left); @@ -245,10 +246,9 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con assert(!region->is_humongous(), "Use region->humongous_start_region() instead"); #endif - // if marking context is valid, 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) { + // 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)) { if (ctx->is_marked(left)) { oop obj = cast_to_oop(left); assert(oopDesc::is_oop(obj), "Should be an object"); @@ -282,10 +282,10 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con return next; } - assert(ctx == nullptr, "Should have returned above"); + assert((ctx == nullptr) || (left >= tams), "Should have returned above"); - // The following code assumes that the region has only parsable objects - assert(ShenandoahGenerationalHeap::heap()->old_generation()->is_parsable(), + // The following code assumes that all data in region at or above left holds parsable objects + assert((left >= tams) || ShenandoahGenerationalHeap::heap()->old_generation()->is_parsable(), "The code that follows expects a parsable heap"); if (starts_object(card_index) && get_first_start(card_index) == 0) { // This card contains a co-initial object; a fortiori, it covers @@ -310,6 +310,32 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con size_t offset = get_last_start(cur_index); // can avoid call via card size arithmetic below instead HeapWord* p = _rs->addr_for_card_index(cur_index) + offset; + if ((ctx != nullptr) && (p < tams)) { + if (ctx->is_marked(p)) { + oop obj = cast_to_oop(p); + assert(oopDesc::is_oop(obj), "Should be an object"); + assert(Klass::is_valid(obj->klass()), "Not a valid klass ptr"); + assert(p + obj->size() > left, "This object should span start of card"); + return p; + } else { + // Object that spans start of card is dead, so should not be scanned + assert((ctx == nullptr) || (left + get_first_start(card_index) >= tams), "Should have handled this case above"); + if (starts_object(card_index)) { + return left + get_first_start(card_index); + } else { + // Spanning object is dead and this card does not start an object, so the start object is in some card that follows + size_t following_card_index = card_index; + do { + following_card_index++; + if (following_card_index > last_relevant_card_index) { + return nullptr; + } + } while (!starts_object(following_card_index)); + return _rs->addr_for_card_index(following_card_index) + get_first_start(following_card_index); + } + } + } + // Recall that we already dealt with the co-initial object case above assert(p < left, "obj should start before left"); // While it is safe to ask an object its size in the loop that diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp index 19940f05bcdca..b79f16d8aeeb4 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp @@ -661,7 +661,8 @@ class ShenandoahCardCluster: public CHeapObj { // 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). - HeapWord* first_object_start(size_t card_index, const ShenandoahMarkingContext* const ctx) const; + HeapWord* first_object_start(size_t card_index, const ShenandoahMarkingContext* const ctx, + HeapWord* tams, const size_t last_relevant_card_index) const; }; // ShenandoahScanRemembered is a concrete class representing the diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp index 651349fd01169..185fd07132517 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp @@ -102,7 +102,7 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou // tams and ctx below are for old generation marking. As such, young gen roots must // consider everything above tams, since it doesn't represent a TAMS for young gen's // SATB marking. - const HeapWord* tams = (ctx == nullptr ? region->bottom() : ctx->top_at_mark_start(region)); + HeapWord* const tams = (ctx == nullptr ? region->bottom() : ctx->top_at_mark_start(region)); NOT_PRODUCT(ShenandoahCardStats stats(whole_cards, card_stats(worker_id));) @@ -174,7 +174,14 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou // This is always the case for large object arrays, which are typically more // common. assert(ctx != nullptr || heap->old_generation()->is_parsable(), "Error"); - HeapWord* p = _scc->first_object_start(dirty_l, ctx); + HeapWord* p = _scc->first_object_start(dirty_l, ctx, tams, dirty_r); + if (p == nullptr) { + // There are no live objects to be scanned in this dirty range. cur_index identifies first card in this + // uninteresting dirty range. At top of next loop iteration, we will either end the looop + // (because cur_index < start_card_index) or we will begin the search for a range of clean cards. + continue; + } + oop obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Not an object"); assert(ctx==nullptr || ctx->is_marked(obj), "Error"); From faaf779af3e58081d5e7b796f5eeb5b9797a0654 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Tue, 16 Sep 2025 23:24:33 +0000 Subject: [PATCH 15/32] Revert extraneous changes --- src/hotspot/share/gc/shared/markBitMap.hpp | 12 ++------- .../share/gc/shared/markBitMap.inline.hpp | 15 +---------- src/hotspot/share/oops/klass.cpp | 9 +------ .../gc/shenandoah/compiler/TestClone.java | 26 +++++++++++++++++-- 4 files changed, 28 insertions(+), 34 deletions(-) diff --git a/src/hotspot/share/gc/shared/markBitMap.hpp b/src/hotspot/share/gc/shared/markBitMap.hpp index 237b289489244..a55a8220910ae 100644 --- a/src/hotspot/share/gc/shared/markBitMap.hpp +++ b/src/hotspot/share/gc/shared/markBitMap.hpp @@ -79,19 +79,11 @@ class MarkBitMap { } // Return the address corresponding to the next marked bit at or after - // "addr", and before "limit", which are required to be non-null. In other - // words, we look for a marked address in the range [addr, limit). If there is no - // such bit, returns "limit". + // "addr", and before "limit", if "limit" is non-null. If there is no + // such bit, returns "limit" if that is non-null, or else "endWord()". inline HeapWord* get_next_marked_addr(const HeapWord* addr, HeapWord* limit) const; - // Return the address corresponding to the last marked bit before - // "addr" but at or after "limit", which are required to be non-null. - // In other words, we look for a marked address in the range [limit, addr). - // If there is no such bit, returns "addr". - inline HeapWord* get_last_marked_addr(HeapWord* limit, - const HeapWord* addr) const; - void print_on(outputStream* st, const char* prefix) const; // Write marks. diff --git a/src/hotspot/share/gc/shared/markBitMap.inline.hpp b/src/hotspot/share/gc/shared/markBitMap.inline.hpp index 3dfe4ce0142b3..cf6cf02cf320c 100644 --- a/src/hotspot/share/gc/shared/markBitMap.inline.hpp +++ b/src/hotspot/share/gc/shared/markBitMap.inline.hpp @@ -36,26 +36,13 @@ inline HeapWord* MarkBitMap::get_next_marked_addr(const HeapWord* const addr, HeapWord* const limit) const { assert(limit != nullptr, "limit must not be null"); - assert(addr != nullptr, "addr must not be null"); - // Round up to bitmap's alignment boundary to start looking + // Round addr up to a possible object boundary to be safe. size_t const addr_offset = addr_to_offset(align_up(addr, HeapWordSize << _shifter)); size_t const limit_offset = addr_to_offset(limit); size_t const nextOffset = _bm.find_first_set_bit(addr_offset, limit_offset); return offset_to_addr(nextOffset); } -inline HeapWord* MarkBitMap::get_last_marked_addr(HeapWord* const limit, - const HeapWord* const addr) const { - assert(limit != nullptr, "limit must not be null"); - assert(addr != nullptr, "addr must not be null"); - size_t const limit_offset = addr_to_offset(limit); - // Round down to bitmap's alignment boundary to start looking back - size_t const addr_offset = addr_to_offset(align_down(addr, HeapWordSize << _shifter)); - size_t const lastOffset = _bm.find_last_set_bit(limit_offset, addr_offset); - return offset_to_addr(lastOffset); -} - - inline void MarkBitMap::mark(HeapWord* addr) { check_mark(addr); _bm.set_bit(addr_to_offset(addr)); diff --git a/src/hotspot/share/oops/klass.cpp b/src/hotspot/share/oops/klass.cpp index 169a07a1cacb4..41ab8e325ab7c 100644 --- a/src/hotspot/share/oops/klass.cpp +++ b/src/hotspot/share/oops/klass.cpp @@ -1110,14 +1110,7 @@ bool Klass::is_valid(Klass* k) { if (!Metaspace::contains(k)) return false; if (!Symbol::is_valid(k->name())) return false; - - // YSR_DEBUGGING - if (SafepointSynchronize::is_at_safepoint()) { - return ClassLoaderDataGraph::is_valid(k->class_loader_data()); - } else { - MutexLocker x(ClassLoaderDataGraph_lock, Mutex::_no_safepoint_check_flag); - return ClassLoaderDataGraph::is_valid(k->class_loader_data()); - } + return ClassLoaderDataGraph::is_valid(k->class_loader_data()); } Method* Klass::method_at_vtable(int index) { diff --git a/test/hotspot/jtreg/gc/shenandoah/compiler/TestClone.java b/test/hotspot/jtreg/gc/shenandoah/compiler/TestClone.java index ae297e5d4312c..0775e5baadd2c 100644 --- a/test/hotspot/jtreg/gc/shenandoah/compiler/TestClone.java +++ b/test/hotspot/jtreg/gc/shenandoah/compiler/TestClone.java @@ -331,8 +331,30 @@ * -XX:-UseCompressedOops * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational * -XX:+ShenandoahVerify - * -Xlog:gc*=info:file=/home/ysr/gc_ysr.log - * -XX:+ShowMessageBoxOnError -XX:+UseCompactObjectHeaders + * TestClone + * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xms1g -Xmx1g + * -XX:-UseCompressedOops + * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational + * -XX:+ShenandoahVerify + * -Xint + * TestClone + * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xms1g -Xmx1g + * -XX:-UseCompressedOops + * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational + * -XX:+ShenandoahVerify + * -XX:-TieredCompilation + * TestClone + * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xms1g -Xmx1g + * -XX:-UseCompressedOops + * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational + * -XX:+ShenandoahVerify + * -XX:TieredStopAtLevel=1 + * TestClone + * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xms1g -Xmx1g + * -XX:-UseCompressedOops + * -XX:+UseShenandoahGC -XX:ShenandoahGCMode=generational + * -XX:+ShenandoahVerify + * -XX:TieredStopAtLevel=4 * TestClone */ From 4f1057ec23590e46e1304584c57cbf1ea12e3256 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Tue, 16 Sep 2025 22:01:12 -0600 Subject: [PATCH 16/32] Add handling for first_object_start() past end of range --- .../share/gc/shenandoah/shenandoahScanRemembered.inline.hpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp index 185fd07132517..7ca905ea08b26 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp @@ -175,7 +175,7 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou // common. assert(ctx != nullptr || heap->old_generation()->is_parsable(), "Error"); HeapWord* p = _scc->first_object_start(dirty_l, ctx, tams, dirty_r); - if (p == nullptr) { + if ((p == nullptr) || (p >= right)) { // There are no live objects to be scanned in this dirty range. cur_index identifies first card in this // uninteresting dirty range. At top of next loop iteration, we will either end the looop // (because cur_index < start_card_index) or we will begin the search for a range of clean cards. @@ -183,7 +183,8 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou } oop obj = cast_to_oop(p); - assert(oopDesc::is_oop(obj), "Not an object"); + assert(oopDesc::is_oop(obj), "Not an object at " PTR_FORMAT ", left: " PTR_FORMAT ", right: " PTR_FORMAT, + p2i(p), p2i(left), p2i(right)); assert(ctx==nullptr || ctx->is_marked(obj), "Error"); // PREFIX: The object that straddles into this range of dirty cards From 84ad6b6172bdfc3908893be7402a1266fcf414b7 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Wed, 17 Sep 2025 17:19:46 +0000 Subject: [PATCH 17/32] Remove troublesome assert that assumes lock is held --- src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 7139d560fef54..ec2ac11fb4526 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -357,7 +357,6 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // 4. Every allocation under TAMS updates the object start array. oop obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Should be an object"); - assert(Klass::is_valid(obj->klass()), "Not a valid klass ptr"); #ifdef ASSERT #define WALK_FORWARD_IN_BLOCK_START true #else From 0583a045bfbcd2918613517ddb1043753a72b116 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Wed, 17 Sep 2025 21:27:02 +0000 Subject: [PATCH 18/32] add explicit typecast to avoid compiler warning message --- test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp index 26727d3c4f900..80da73af3e60f 100644 --- a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp +++ b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp @@ -221,7 +221,7 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { HeapWord* heap_limit = end_of_my_heap; MarkBitMapAssertEqual(sentinel, heap_limit); HeapWord* prev_marked = end_of_my_heap + 1;; - for (int i = weakly_marked_objects - 1; i >= 0; i--) { + for (int i = (int) weakly_marked_objects - 1; i >= 0; i--) { // to be renamed get_prev_marked_addr() prev_marked = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked - 1); MarkBitMapAssertEqual(prev_marked, weakly_marked_addresses[i]); From 7578484ddae4b1d407c02f281e343fc1bdda5b06 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Wed, 17 Sep 2025 23:12:03 +0000 Subject: [PATCH 19/32] disable for debug build, alphabetic order for includes --- .../shenandoah/test_shenandoahMarkBitMap.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp index 80da73af3e60f..081262ad93bf5 100644 --- a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp +++ b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp @@ -21,19 +21,21 @@ * questions. */ -#include "unittest.hpp" -#include "memory/memRegion.hpp" #include "gc/shenandoah/shenandoahHeap.inline.hpp" #include "gc/shenandoah/shenandoahMarkBitMap.hpp" #include "gc/shenandoah/shenandoahMarkBitMap.inline.hpp" -#include "utilities/ostream.hpp" -#include "utilities/vmassert_uninstall.hpp" BEGIN_ALLOW_FORBIDDEN_FUNCTIONS #include END_ALLOW_FORBIDDEN_FUNCTIONS + +#include "memory/memRegion.hpp" +#include "unittest.hpp" + +#include "utilities/ostream.hpp" #include "utilities/vmassert_reinstall.hpp" +#include "utilities/vmassert_uninstall.hpp" // These tests will all be skipped (unless Shenandoah becomes the default // collector). To execute these tests, you must enable Shenandoah, which @@ -49,14 +51,17 @@ END_ALLOW_FORBIDDEN_FUNCTIONS // interval - you can expect trouble. These tests will also not run in a build // with asserts enabled because they use APIs that expect to run on a safepoint. -// I may have to disable tests if this is a debug build, but let's try -// without that constraint first -// +#ifdef ASSERT +#define SKIP_IF_NOT_SHENANDOAH() \ + std::cout << "skipped (debug build)\n"; \ + return; +#else #define SKIP_IF_NOT_SHENANDOAH() \ if (!UseShenandoahGC) { \ std::cout << "skipped\n"; \ return; \ } +#endif static bool _success; static size_t _assertion_failures; From 9c87c2fd1c21fc7ea8f34f9f7c5c89aff8584fe6 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Wed, 17 Sep 2025 23:23:54 +0000 Subject: [PATCH 20/32] fix white space --- src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp | 1 - .../share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp | 1 - .../hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp | 4 ++-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp index c1901fd93f4e5..2424e180b3078 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp @@ -120,7 +120,6 @@ class ShenandoahMarkBitMap { // comments should clarify the ranges examined to find the bit, and what value is returned // in case searched bit is not found // rename flip template parameter as match_one_bit (false means match_zero bit) - #endif diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp index 3236246a65cb7..4f9615b367dd9 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp @@ -213,7 +213,6 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_last_bit_impl(idx_t bm_word_t cword = (map(index) ^ flip) << shift; // After this shift, the highest order bits correspond to r_index. - // With 64-bit words, // We give special handling if either of the two most significant bits (Weak or Strong) is set. With 64-bit // words, the mask of interest is 0xc000_0000_0000_0000. Symbolically, this constant is represented by: const bm_word_t first_object_mask = ((bm_word_t) 0x3) << (BitsPerWord - 2); diff --git a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp index 081262ad93bf5..7fdcc10339d96 100644 --- a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp +++ b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp @@ -192,7 +192,7 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { mbm.clear_range_large(heap_descriptor); verify_bitmap_is_empty((HeapWord*) my_heap_memory, heap_size_words, &mbm); - + HeapWord* weakly_marked_addresses[] = { (HeapWord*) &my_heap_memory[13], (HeapWord*) &my_heap_memory[14], @@ -277,7 +277,7 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { MarkBitMapAssertTrue(upgraded); } verify_bitmap_is_strongly_marked(&mbm, upgraded_weakly_marked_addresses, upgraded_weakly_marked_objects); - + HeapWord* all_marked_addresses[] = { (HeapWord*) &my_heap_memory[8], /* strongly marked */ (HeapWord*) &my_heap_memory[13], /* weakly marked */ From 349818d99f2e074ca6205c7c682d2f2cea180a60 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Thu, 18 Sep 2025 16:51:13 +0000 Subject: [PATCH 21/32] Fix order of include files --- src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp index 4f9615b367dd9..2b2e4fcb107e7 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp @@ -29,8 +29,8 @@ #include "gc/shenandoah/shenandoahMarkBitMap.hpp" #include "runtime/atomicAccess.hpp" -#include "utilities/count_trailing_zeros.hpp" #include "utilities/count_leading_zeros.hpp" +#include "utilities/count_trailing_zeros.hpp" inline size_t ShenandoahMarkBitMap::address_to_index(const HeapWord* addr) const { return (pointer_delta(addr, _covered.start()) << 1) >> _shift; From 76979421cf1ad059dbc1d51a5cd1054a628fe61d Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Fri, 26 Sep 2025 09:14:07 +0000 Subject: [PATCH 22/32] cleanup code for review --- .../gc/shenandoah/shenandoahMarkBitMap.cpp | 11 +----- .../gc/shenandoah/shenandoahMarkBitMap.hpp | 37 ++----------------- .../shenandoahMarkBitMap.inline.hpp | 16 ++------ .../shenandoahMarkingContext.inline.hpp | 2 +- .../shenandoah/test_shenandoahMarkBitMap.cpp | 9 ++--- 5 files changed, 14 insertions(+), 61 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp index 9cf553e9ba8ac..5318f38d8ef67 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp @@ -57,14 +57,7 @@ bool ShenandoahMarkBitMap::is_bitmap_clear_range(const HeapWord* start, const He return (result == end); } -#ifdef KELVIN_THINKING_OUT_LOUD -// This is new functionality introduced by this PR. I want to change -// the name to get_prev_marked_addr. I want to clarify that we search over the range [limit, addr] and -// that the Sentinel value addr+1 denotes no previous marked object found. -// This comment is present in the .hpp file. -#endif - -HeapWord* ShenandoahMarkBitMap::get_last_marked_addr(const HeapWord* limit, +HeapWord* ShenandoahMarkBitMap::get_prev_marked_addr(const HeapWord* limit, const HeapWord* addr) const { #ifdef ASSERT ShenandoahHeap* heap = ShenandoahHeap::heap(); @@ -79,7 +72,7 @@ HeapWord* ShenandoahMarkBitMap::get_last_marked_addr(const HeapWord* limit, // Round addr down to a possible object boundary to be safe. size_t const addr_offset = address_to_index(align_down(addr, HeapWordSize << LogMinObjAlignment)); size_t const limit_offset = address_to_index(limit); - size_t const last_offset = get_last_one_offset(limit_offset, addr_offset); + size_t const last_offset = get_prev_one_offset(limit_offset, addr_offset); // cast required to remove const-ness of the value pointed to. We won't modify that object, but my caller might. return (last_offset > addr_offset)? (HeapWord*) addr + 1: index_to_address(last_offset); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp index 2424e180b3078..e0626c88a5f68 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp @@ -112,17 +112,6 @@ class ShenandoahMarkBitMap { return raw_to_words_align_down(bit); } -#ifdef KELVIN_THINKING_OUT_LOUD - // kelvin wants to redefine the flip parameter in the following, after he understands the code - // better. the current comments are a bit cryptic. - - // naming should be get_first_matching_bit() and get_last_matching_bit() - // comments should clarify the ranges examined to find the bit, and what value is returned - // in case searched bit is not found - // rename flip template parameter as match_one_bit (false means match_zero bit) - -#endif - // Helper for get_next_{zero,one}_bit variants. // - flip designates whether searching for 1s or 0s. Must be one of // find_{zeros,ones}_flip. @@ -130,25 +119,17 @@ class ShenandoahMarkBitMap { template inline idx_t get_next_bit_impl(idx_t l_index, idx_t r_index) const; - // Helper for get_last_{zero,one}_bit variants. + // Helper for get_prev_{zero,one}_bit variants. // - flip designates whether searching for 1s or 0s. Must be one of // find_{zeros,ones}_flip. // - aligned_left is true if l_index is a priori on a bm_word_t boundary. template - inline idx_t get_last_bit_impl(idx_t l_index, idx_t r_index) const; - -#ifdef KELVIN_THINKING_OUT_LOUD - // kelvin wants to rename get_next_one_offset() as get_first_one_offset - // would like search space to be specified as (l_index, r_index]. Return l_index if not found. - - // does this require changes to implementation and invocations? TBD -#endif - + 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; // Search for last one in the range [l_index, r_index). Return r_index if not found. - inline idx_t get_last_one_offset (idx_t l_index, idx_t r_index) const; + 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); @@ -196,18 +177,8 @@ class ShenandoahMarkBitMap { HeapWord* get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const; -#ifdef KELVIN_THINKING_OUT_LOAD - // I haven't changed this code yet, but I'm thinking the following change would also be appropriate: - // get_next_marked_addr() should be renamed get_first_marked_addr - // The search domain for get_first_marked_addr should be (addr, limit) and not-found sentinel is limit - // (looking for first marked object that comes after addr) - - // Might have to adjust the invocations of these two functions, or maybe they already behave - // this way and the commentaries are wrong. -#endif - // Return the last marked address in the range [limit, addr], or addr+1 if none found. - HeapWord* get_last_marked_addr(const HeapWord* limit, + HeapWord* get_prev_marked_addr(const HeapWord* limit, const HeapWord* addr) const; bm_word_t inverted_bit_mask_for_range(idx_t beg, idx_t end) const; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp index 2b2e4fcb107e7..78733f50b9b84 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp @@ -136,9 +136,6 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_bit_impl(idx_t // Get the word containing l_index, and shift out low bits. idx_t index = to_words_align_down(l_index); bm_word_t cword = (map(index) ^ flip) >> bit_in_word(l_index); -#ifdef KELVIN_THINKING_OUT_LOUD - // the following test should look at (cword & 0x3) -#endif if ((cword & 1) != 0) { // The first bit is similarly often interesting. When it matters // (density or features of the calling algorithm make it likely @@ -175,15 +172,8 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_bit_impl(idx_t return r_index; } -#ifdef KELVIN_THINKING_OUT_LOUND -// Make cleaer that we find that last_bit in the range [l_index, r_index), returning r_index if none found - -// kelvin thinks the implementation is currently wrong, returning l_index for failure to find one. - -// kelvin also prefers to use all caps for template parameters. -#endif template -inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_last_bit_impl(idx_t l_index, idx_t r_index) const { +inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_prev_bit_impl(idx_t l_index, idx_t r_index) const { STATIC_ASSERT(flip == find_ones_flip || flip == find_zeros_flip); verify_range(l_index, r_index); assert(!aligned_left || is_aligned(l_index, BitsPerWord), "l_index not aligned"); @@ -271,8 +261,8 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_one_offset(idx return get_next_bit_impl(l_offset, r_offset); } -inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_last_one_offset(idx_t l_offset, idx_t r_offset) const { - return get_last_bit_impl(l_offset, r_offset); +inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_prev_one_offset(idx_t l_offset, idx_t r_offset) const { + return get_prev_bit_impl(l_offset, r_offset); } // Returns a bit mask for a range of bits [beg, end) within a single word. Each diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp index f3853a1936a1f..fe98413a8ccb0 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.inline.hpp @@ -73,7 +73,7 @@ inline HeapWord* ShenandoahMarkingContext::get_next_marked_addr(const HeapWord* } inline HeapWord* ShenandoahMarkingContext::get_prev_marked_addr(const HeapWord* limit, const HeapWord* start) const { - return _mark_bit_map.get_last_marked_addr(limit, start); + return _mark_bit_map.get_prev_marked_addr(limit, start); } inline bool ShenandoahMarkingContext::allocated_after_mark_start(oop obj) const { diff --git a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp index 7fdcc10339d96..3dbb7c621226a 100644 --- a/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp +++ b/test/hotspot/gtest/gc/shenandoah/test_shenandoahMarkBitMap.cpp @@ -146,8 +146,7 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { HeapWord* prev_marked = end_of_heap_memory + 1; for (int i = (int) all_marked_objects - 1; i >= 0; i--) { - // to be renamed get_prev_marked_addr() - prev_marked = mbm->get_last_marked_addr(&heap_memory[0], prev_marked - 1); + prev_marked = mbm->get_prev_marked_addr(&heap_memory[0], prev_marked - 1); MarkBitMapAssertEqual(prev_marked, all_marked_addresses[i]); MarkBitMapAssertTrue(mbm->is_marked(prev_marked)); if (is_strongly_marked_object[i]) { @@ -158,7 +157,7 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { } } // We expect no more marked addresses to be found. should return prev_marked. - sentinel = mbm->get_last_marked_addr(&heap_memory[0], prev_marked - 1); + sentinel = mbm->get_prev_marked_addr(&heap_memory[0], prev_marked - 1); MarkBitMapAssertEqual(sentinel, prev_marked); } @@ -228,14 +227,14 @@ class ShenandoahMarkBitMapTest: public ::testing::Test { HeapWord* prev_marked = end_of_my_heap + 1;; for (int i = (int) weakly_marked_objects - 1; i >= 0; i--) { // to be renamed get_prev_marked_addr() - prev_marked = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked - 1); + prev_marked = mbm.get_prev_marked_addr(&my_heap_memory[0], prev_marked - 1); MarkBitMapAssertEqual(prev_marked, weakly_marked_addresses[i]); MarkBitMapAssertTrue(mbm.is_marked(prev_marked)); MarkBitMapAssertTrue(mbm.is_marked_weak(prev_marked)); MarkBitMapAssertTrue(!mbm.is_marked_strong(prev_marked)); } // We expect no more marked addresses to be found. should return prev_marked. - sentinel = mbm.get_last_marked_addr(&my_heap_memory[0], prev_marked - 1); + sentinel = mbm.get_prev_marked_addr(&my_heap_memory[0], prev_marked - 1); // MarkBitMapAssertEqual(sentinel, prev_marked); MarkBitMapAssertEqual(sentinel, prev_marked); verify_bitmap_is_weakly_marked(&mbm, weakly_marked_addresses, weakly_marked_objects); From 668e8615630d251af62d5a3b82c0cd6fd31c5d24 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Fri, 3 Oct 2025 00:17:04 +0000 Subject: [PATCH 23/32] fix idiosyncratic formatting --- src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp index e0626c88a5f68..b1436c954ad03 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp @@ -126,12 +126,12 @@ class ShenandoahMarkBitMap { template 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; + inline idx_t get_next_one_offset(idx_t l_index, idx_t r_index) const; // 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; + 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); + void clear_large_range(idx_t beg, idx_t end); // Verify bit is less than size(). void verify_index(idx_t bit) const NOT_DEBUG_RETURN; From 80198abe5d06c3532d9a43a53691376e990ed45f Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Mon, 20 Oct 2025 22:19:06 +0000 Subject: [PATCH 24/32] Fixup handling of weakly marked objects in remembered set --- .../shenandoah/shenandoahScanRemembered.cpp | 43 +++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index ec2ac11fb4526..d7644caf192c6 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -249,15 +249,23 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // 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)) { - if (ctx->is_marked(left)) { + if (ctx->is_marked_strong(left)) { oop obj = cast_to_oop(left); assert(oopDesc::is_oop(obj), "Should be an object"); return left; } // get the previous marked object, if any if (region->bottom() < left) { + // Whether this region was previously marked as young and was subsequently promoted in place, or was marked as old. + // In the case that this region was most recently marked as young, the fact that this region has been promoted + // in place denotes that final mark (Young) has copmleted. In the case that this region was most recently marked + // as old, the fact that (ctx != nullptr) denotes that old marking has completed. Otherwise, ctx would equal null. + // + // Given that marking has completed, if this object is only marked weakly, then this object is dead. Its memory will + // be reclaimed momentarily. Given that this object is dead, its class may also be reclaimed. Therefore, we cannot + // rely on its size() method, and we should not scan its pointers. HeapWord* prev = ctx->get_prev_marked_addr(region->bottom(), left); - if (prev <= left) { + if ((prev <= left) && ctx->is_marked_strong(prev)) { oop obj = cast_to_oop(prev); assert(oopDesc::is_oop(obj), "Should be an object"); HeapWord* obj_end = prev + obj->size(); @@ -266,19 +274,27 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } } } - // Either prev >= left (no previous object found), or the previous object that was found ends before my card range begins. - // In eiher case, find the next marked object if any on this card - assert(!ctx->is_marked(left), "Was dealt with above"); + // Either prev >= left (no previous object found), or the previous object that was found ends before my card range begins, + // or the previously object that was found was only weakly marked, so it should not be scanned. In all of these cases, + // find the next strongly marked object if any on this card + assert(!ctx->is_marked_strong(left), "Was dealt with above"); HeapWord* right = MIN2(region->top(), ctx->top_at_mark_start(region)); assert(right > left, "We don't expect to be examining cards above the smaller of TAMS or top"); - HeapWord* next = ctx->get_next_marked_addr(left, right); + HeapWord* next = left; + do { + next = ctx->get_next_marked_addr(next, right); + } while ((next < right) && !ctx->is_marked_strong(next)); #ifdef ASSERT if (next < right) { oop obj = cast_to_oop(next); assert(oopDesc::is_oop(obj), "Should be an object"); } #endif - // Note: returned value may point beyond this card's range of memory, and may not point to an allocated object. + // If top_at_mark_start(region) is within this card's range, we will return its value. If the returned value + // is less than region->top(), then the returned value represents an object that should be scanned. Otherwise, + // the returned value equals region->top() and will not be scanned. There are no races with ongoing allocations + // within old generation because this function is only called during concurrent marking and during concurrent + // update references. return next; } @@ -288,8 +304,7 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con assert((left >= tams) || ShenandoahGenerationalHeap::heap()->old_generation()->is_parsable(), "The code that follows expects a parsable heap"); if (starts_object(card_index) && get_first_start(card_index) == 0) { - // This card contains a co-initial object; a fortiori, it covers - // also the case of a card being the first in a region. + // This card contains a co-initial object; a fortiori, it covers also the case of a card being the first in a region. assert(oopDesc::is_oop(cast_to_oop(left)), "Should be an object"); return left; } @@ -311,14 +326,15 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // can avoid call via card size arithmetic below instead HeapWord* p = _rs->addr_for_card_index(cur_index) + offset; if ((ctx != nullptr) && (p < tams)) { - if (ctx->is_marked(p)) { + if (ctx->is_marked_strong(p)) { oop obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Should be an object"); assert(Klass::is_valid(obj->klass()), "Not a valid klass ptr"); assert(p + obj->size() > left, "This object should span start of card"); return p; } else { - // Object that spans start of card is dead, so should not be scanned + // Object that spans start of card is dead, so should not be scanned. + // From above, we know that if (ctx != nullptr), left >= tams. Therefore, left + offset >= tams. assert((ctx == nullptr) || (left + get_first_start(card_index) >= tams), "Should have handled this case above"); if (starts_object(card_index)) { return left + get_first_start(card_index); @@ -336,6 +352,9 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } } + // p points to first object that precedes the start of card card_index. The object is "alive". + assert((ctx == nullptr) || (p >= tams), "Sanity"); + // Recall that we already dealt with the co-initial object case above assert(p < left, "obj should start before left"); // While it is safe to ask an object its size in the loop that @@ -360,7 +379,7 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con #ifdef ASSERT #define WALK_FORWARD_IN_BLOCK_START true #else -#define WALK_FORWARD_IN_BLOCK_START true +#define WALK_FORWARD_IN_BLOCK_START false #endif // ASSERT while (WALK_FORWARD_IN_BLOCK_START && p + obj->size() < left) { p += obj->size(); From e16ea23183b25af31de18da32940e6ec55d86bfe Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Sat, 1 Nov 2025 05:34:47 +0000 Subject: [PATCH 25/32] fix bugs in implementation of weakly referenced object handling --- .../gc/shenandoah/shenandoahMarkingContext.hpp | 2 +- .../gc/shenandoah/shenandoahScanRemembered.cpp | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp index a5ad5008b6a6f..d8e0c74ea4e94 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp @@ -71,7 +71,7 @@ class ShenandoahMarkingContext : public CHeapObj { inline HeapWord* get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const; // Return address of the last marked object in range [limit, start], returning start+1 if no marked object found - inline HeapWord* get_prev_marked_addr(const HeapWord* limit, const HeapWord* addr) const; + inline HeapWord* get_prev_marked_addr(const HeapWord* limit, const HeapWord* start) const; inline bool allocated_after_mark_start(const oop obj) const; inline bool allocated_after_mark_start(const HeapWord* addr) const; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index d7644caf192c6..3d06efdc16457 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -256,15 +256,14 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } // get the previous marked object, if any if (region->bottom() < left) { - // Whether this region was previously marked as young and was subsequently promoted in place, or was marked as old. // In the case that this region was most recently marked as young, the fact that this region has been promoted - // in place denotes that final mark (Young) has copmleted. In the case that this region was most recently marked + // in place denotes that final mark (Young) has completed. In the case that this region was most recently marked // as old, the fact that (ctx != nullptr) denotes that old marking has completed. Otherwise, ctx would equal null. - // + + HeapWord* prev = ctx->get_prev_marked_addr(region->bottom(), left); // Given that marking has completed, if this object is only marked weakly, then this object is dead. Its memory will - // be reclaimed momentarily. Given that this object is dead, its class may also be reclaimed. Therefore, we cannot + // be reclaimed momentarily. Since this object is dead, its class may also be reclaimed. Therefore, we cannot // rely on its size() method, and we should not scan its pointers. - HeapWord* prev = ctx->get_prev_marked_addr(region->bottom(), left); if ((prev <= left) && ctx->is_marked_strong(prev)) { oop obj = cast_to_oop(prev); assert(oopDesc::is_oop(obj), "Should be an object"); @@ -280,9 +279,9 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con assert(!ctx->is_marked_strong(left), "Was dealt with above"); HeapWord* right = MIN2(region->top(), ctx->top_at_mark_start(region)); assert(right > left, "We don't expect to be examining cards above the smaller of TAMS or top"); - HeapWord* next = left; + HeapWord* next = left - 1; do { - next = ctx->get_next_marked_addr(next, right); + next = ctx->get_next_marked_addr(next + 1, right); } while ((next < right) && !ctx->is_marked_strong(next)); #ifdef ASSERT if (next < right) { From 643cdfd650f488305e1ce2f00ff9f78aac91ba1c Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Wed, 5 Nov 2025 02:21:13 +0000 Subject: [PATCH 26/32] Revert "Fixup handling of weakly marked objects in remembered set" This reverts commit 80198abe5d06c3532d9a43a53691376e990ed45f. --- .../shenandoah/shenandoahScanRemembered.cpp | 40 ++++++------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 04dcf6b0e1ef2..411547eb566f7 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -261,22 +261,18 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // 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)) { - if (ctx->is_marked_strong(left)) { + if (ctx->is_marked(left)) { oop obj = cast_to_oop(left); assert(oopDesc::is_oop(obj), "Should be an object"); return left; } // get the previous marked object, if any if (region->bottom() < left) { - // In the case that this region was most recently marked as young, the fact that this region has been promoted - // in place denotes that final mark (Young) has completed. In the case that this region was most recently marked - // as old, the fact that (ctx != nullptr) denotes that old marking has completed. Otherwise, ctx would equal null. - + // In the case that this region was most recently marked as young, the fact that this region has been promoted in place + // denotes that final mark (Young) has completed. In the case that this region was most recently marked as old, the + // fact that (ctx != nullptr) denotes that old marking has completed. Otherwise, ctx would equal null. HeapWord* prev = ctx->get_prev_marked_addr(region->bottom(), left); - // Given that marking has completed, if this object is only marked weakly, then this object is dead. Its memory will - // be reclaimed momentarily. Since this object is dead, its class may also be reclaimed. Therefore, we cannot - // rely on its size() method, and we should not scan its pointers. - if ((prev <= left) && ctx->is_marked_strong(prev)) { + if (prev <= left) { oop obj = cast_to_oop(prev); assert(oopDesc::is_oop(obj), "Should be an object"); HeapWord* obj_end = prev + obj->size(); @@ -285,27 +281,19 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } } } - // Either prev >= left (no previous object found), or the previous object that was found ends before my card range begins, - // or the previously object that was found was only weakly marked, so it should not be scanned. In all of these cases, - // find the next strongly marked object if any on this card - assert(!ctx->is_marked_strong(left), "Was dealt with above"); + // Either prev >= left (no previous object found), or the previous object that was found ends before my card range begins. + // In eiher case, find the next marked object if any on this card + assert(!ctx->is_marked(left), "Was dealt with above"); HeapWord* right = MIN2(region->top(), ctx->top_at_mark_start(region)); assert(right > left, "We don't expect to be examining cards above the smaller of TAMS or top"); - HeapWord* next = left - 1; - do { - next = ctx->get_next_marked_addr(next + 1, right); - } while ((next < right) && !ctx->is_marked_strong(next)); + HeapWord* next = ctx->get_next_marked_addr(left, right); #ifdef ASSERT if (next < right) { oop obj = cast_to_oop(next); assert(oopDesc::is_oop(obj), "Should be an object"); } #endif - // If top_at_mark_start(region) is within this card's range, we will return its value. If the returned value - // is less than region->top(), then the returned value represents an object that should be scanned. Otherwise, - // the returned value equals region->top() and will not be scanned. There are no races with ongoing allocations - // within old generation because this function is only called during concurrent marking and during concurrent - // update references. + // Note: returned value may point beyond this card's range of memory, and may not point to an allocated object. return next; } @@ -337,15 +325,14 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // can avoid call via card size arithmetic below instead HeapWord* p = _rs->addr_for_card_index(cur_index) + offset; if ((ctx != nullptr) && (p < tams)) { - if (ctx->is_marked_strong(p)) { + if (ctx->is_marked(p)) { oop obj = cast_to_oop(p); assert(oopDesc::is_oop(obj), "Should be an object"); assert(Klass::is_valid(obj->klass()), "Not a valid klass ptr"); assert(p + obj->size() > left, "This object should span start of card"); return p; } else { - // Object that spans start of card is dead, so should not be scanned. - // From above, we know that if (ctx != nullptr), left >= tams. Therefore, left + offset >= tams. + // Object that spans start of card is dead, so should not be scanned assert((ctx == nullptr) || (left + get_first_start(card_index) >= tams), "Should have handled this case above"); if (starts_object(card_index)) { return left + get_first_start(card_index); @@ -363,9 +350,6 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } } - // p points to first object that precedes the start of card card_index. The object is "alive". - assert((ctx == nullptr) || (p >= tams), "Sanity"); - // Recall that we already dealt with the co-initial object case above assert(p < left, "obj should start before left"); // While it is safe to ask an object its size in the loop that From 637c1775d610d3755b5950791de76bf405aacb2c Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Fri, 7 Nov 2025 06:08:01 +0000 Subject: [PATCH 27/32] fix up comments and simplify API for ShenandoahScanRemembered::first_object_start() --- .../shenandoah/shenandoahScanRemembered.cpp | 26 +++++++------------ .../shenandoah/shenandoahScanRemembered.hpp | 25 +++++++++++++----- .../shenandoahScanRemembered.inline.hpp | 3 ++- 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 411547eb566f7..29d5d5ce2611a 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -233,17 +233,6 @@ size_t ShenandoahCardCluster::get_last_start(size_t card_index) const { return _object_starts[card_index].offsets.last; } -// Given a card_index, return the starting address of the first object in the heap -// that intersects with this card. This must be a valid, parsable object, and must -// be the first such object that intersects with this card. The object may start before, -// at, or after the start of the card, and may end in or after the card. If no -// such object exists, a null value is returned. -// Expects to be called for a card in an region affiliated with the old generation in -// generational heap, otherwise behavior is undefined. -// 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). HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, const ShenandoahMarkingContext* const ctx, HeapWord* tams, const size_t last_relevant_card_index) const { @@ -259,7 +248,9 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con #endif // 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 + // intersects with this card. If no such object exists, we return the first marked object that follows the start + // of this card's memory range if such an object is found at or before last_relevant_card_index. If there are no + // marked objects in this range, we return nullptr. if ((ctx != nullptr) && (left < tams)) { if (ctx->is_marked(left)) { oop obj = cast_to_oop(left); @@ -282,19 +273,20 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } } // Either prev >= left (no previous object found), or the previous object that was found ends before my card range begins. - // In eiher case, find the next marked object if any on this card + // In eiher case, find the next marked object if any on this or a following card assert(!ctx->is_marked(left), "Was dealt with above"); - HeapWord* right = MIN2(region->top(), ctx->top_at_mark_start(region)); + HeapWord* right = MIN2(region->top(), tams); assert(right > left, "We don't expect to be examining cards above the smaller of TAMS or top"); HeapWord* next = ctx->get_next_marked_addr(left, right); #ifdef ASSERT if (next < right) { oop obj = cast_to_oop(next); assert(oopDesc::is_oop(obj), "Should be an object"); + return next; + } else { + return nullptr; } #endif - // Note: returned value may point beyond this card's range of memory, and may not point to an allocated object. - return next; } assert((ctx == nullptr) || (left >= tams), "Should have returned above"); @@ -317,7 +309,7 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } // cur_index should start an object: we should not have walked // past the left end of the region. - assert(cur_index >= 0 && (cur_index <= (ssize_t)card_index), "Error"); + assert(cur_index >= 0 && (cur_index <= (ssize_t) card_index), "Error"); assert(region->bottom() <= _rs->addr_for_card_index(cur_index), "Fell off the bottom of containing region"); assert(starts_object(cur_index), "Error"); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp index 661ef09360eab..0f3e711aa3372 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp @@ -652,17 +652,30 @@ class ShenandoahCardCluster: public CHeapObj { size_t get_last_start(size_t card_index) const; - // Given a card_index, return the starting address of the first object in the heap - // that intersects with this card. This must be a valid, parsable object, and must + // Given a card_index, return the starting address of the first live object in the heap + // that intersects with or follows this card. This must be a valid, parsable object, and must // be the first such object that intersects with this card. The object may start before, - // at, or after the start of the card, and may end in or after the card. If no - // such object exists, a null value is returned. - // Expects to be called for a card in an region affiliated with the old generation in + // at, or after the start of the card identified by card_index, and may end in or after the card. + // + // The tams argument represents top for the enclosing region at the start of the most recently + // initiated concurrent old marking effort. If ctx is non-null, we use the marking context to identify + // marked objects below tams. Above tams, we know that every object is marked and that the memory is + // parsable (so we can add an object's size to its address to find the next object). If ctx is null, + // we use crossing maps to find where object's start, and use object sizes to walk individual objects. + // The region must be parsable if ctx is null. + // + // The last_relevant_card_index represents an upper bound on how far we look in the forward direction + // for the first object in the heap that intersects or follows this card. If there are no live objects + // found within the range of cards identified by [card_index, last_relevant_card_index], this function + // returns nullptr. + // + // Expects to be called for a card in a region affiliated with the old generation of the // generational heap, otherwise behavior is undefined. + // // 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). + // or unloaded objects have been coalesced and filled. Coalesce and fill updates the crossing maps. HeapWord* first_object_start(size_t card_index, const ShenandoahMarkingContext* const ctx, HeapWord* tams, const size_t last_relevant_card_index) const; }; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp index cfd365a617959..37461e3b7ea3a 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp @@ -175,7 +175,8 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou // common. assert(ctx != nullptr || heap->old_generation()->is_parsable(), "Error"); HeapWord* p = _scc->first_object_start(dirty_l, ctx, tams, dirty_r); - if ((p == nullptr) || (p >= right)) { + assert((p == nullptr) || (p < right), "No first object found is denoted by nullptr"); + if (p == nullptr) { // There are no live objects to be scanned in this dirty range. cur_index identifies first card in this // uninteresting dirty range. At top of next loop iteration, we will either end the looop // (because cur_index < start_card_index) or we will begin the search for a range of clean cards. From 0e2120b85743dede242b103694b1217cf27e4743 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Fri, 7 Nov 2025 11:20:44 -0700 Subject: [PATCH 28/32] consider last_relevant_card in determining right-most address --- src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 29d5d5ce2611a..6df831a1d7cb2 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -275,7 +275,7 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // Either prev >= left (no previous object found), or the previous object that was found ends before my card range begins. // In eiher case, find the next marked object if any on this or a following card assert(!ctx->is_marked(left), "Was dealt with above"); - HeapWord* right = MIN2(region->top(), tams); + HeapWord* right = MIN3(region->top(), tams, _rs->addr_for_card_index(last_relevant_card_index + 1)); assert(right > left, "We don't expect to be examining cards above the smaller of TAMS or top"); HeapWord* next = ctx->get_next_marked_addr(left, right); #ifdef ASSERT From 29f5d42c48f9fc198af35cd207bae69a464c027a Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Sat, 8 Nov 2025 19:49:02 +0000 Subject: [PATCH 29/32] Refinements and debugging --- .../gc/shenandoah/shenandoahMarkBitMap.hpp | 3 +- .../shenandoah/shenandoahScanRemembered.cpp | 29 +++++++++++++------ .../shenandoah/shenandoahScanRemembered.hpp | 9 +++--- .../shenandoahScanRemembered.inline.hpp | 20 +++++++++++-- 4 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp index 71fc087034ad4..349148dd41f94 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp @@ -172,8 +172,7 @@ class ShenandoahMarkBitMap { bool is_bitmap_clear_range(const HeapWord* start, const HeapWord* end) const; - // Return the first marked address in the range [addr, limit), or limit - // if none found. + // Return the first marked address in the range [addr, limit), or limit if none found. HeapWord* get_next_marked_addr(const HeapWord* addr, const HeapWord* limit) const; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index 6df831a1d7cb2..ecbb822abf6c9 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -234,7 +234,7 @@ size_t ShenandoahCardCluster::get_last_start(size_t card_index) const { } HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, const ShenandoahMarkingContext* const ctx, - HeapWord* tams, const size_t last_relevant_card_index) const { + HeapWord* tams, HeapWord* end_range_of_interest) const { HeapWord* left = _rs->addr_for_card_index(card_index); ShenandoahHeapRegion* region = ShenandoahHeap::heap()->heap_region_containing(left); @@ -247,6 +247,15 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con assert(!region->is_humongous(), "Use region->humongous_start_region() instead"); #endif + HeapWord* right = MIN2(region->top(), end_range_of_interest); + HeapWord* end_of_search_next = MIN2(right, tams); + size_t 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--; + } + assert(card_index <= last_relevant_card_index, "sanity: card_index: %zu, last_relevant: %zu, left: " PTR_FORMAT + ", end_of_range: " PTR_FORMAT, card_index, last_relevant_card_index, p2i(left), p2i(end_range_of_interest)); + // 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. If no such object exists, we return the first marked object that follows the start // of this card's memory range if such an object is found at or before last_relevant_card_index. If there are no @@ -262,8 +271,8 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // In the case that this region was most recently marked as young, the fact that this region has been promoted in place // denotes that final mark (Young) has completed. In the case that this region was most recently marked as old, the // fact that (ctx != nullptr) denotes that old marking has completed. Otherwise, ctx would equal null. - HeapWord* prev = ctx->get_prev_marked_addr(region->bottom(), left); - if (prev <= left) { + HeapWord* prev = ctx->get_prev_marked_addr(region->bottom(), left - 1); + if (prev < left) { oop obj = cast_to_oop(prev); assert(oopDesc::is_oop(obj), "Should be an object"); HeapWord* obj_end = prev + obj->size(); @@ -275,10 +284,9 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con // Either prev >= left (no previous object found), or the previous object that was found ends before my card range begins. // In eiher case, find the next marked object if any on this or a following card assert(!ctx->is_marked(left), "Was dealt with above"); - HeapWord* right = MIN3(region->top(), tams, _rs->addr_for_card_index(last_relevant_card_index + 1)); assert(right > left, "We don't expect to be examining cards above the smaller of TAMS or top"); - HeapWord* next = ctx->get_next_marked_addr(left, right); -#ifdef ASSERT + HeapWord* next = ctx->get_next_marked_addr(left, end_of_search_next); + // If end_of_search_next < right, we may return tams here, which is "marked" by default if (next < right) { oop obj = cast_to_oop(next); assert(oopDesc::is_oop(obj), "Should be an object"); @@ -286,7 +294,6 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con } else { return nullptr; } -#endif } assert((ctx == nullptr) || (left >= tams), "Should have returned above"); @@ -322,11 +329,13 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con assert(oopDesc::is_oop(obj), "Should be an object"); assert(Klass::is_valid(obj->klass()), "Not a valid klass ptr"); assert(p + obj->size() > left, "This object should span start of card"); + assert(p < right, "Result must precede right"); return p; } else { // Object that spans start of card is dead, so should not be scanned assert((ctx == nullptr) || (left + get_first_start(card_index) >= tams), "Should have handled this case above"); if (starts_object(card_index)) { + assert(left + get_first_start(card_index) < right, "Result must precede right"); return left + get_first_start(card_index); } else { // Spanning object is dead and this card does not start an object, so the start object is in some card that follows @@ -337,6 +346,8 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con return nullptr; } } while (!starts_object(following_card_index)); + assert(_rs->addr_for_card_index(following_card_index) + get_first_start(following_card_index), + "Result must precede right"); return _rs->addr_for_card_index(following_card_index) + get_first_start(following_card_index); } } @@ -377,8 +388,8 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con guarantee(false, "Should never need forward walk in block start"); } #undef WALK_FORWARD_IN_BLOCK_START - guarantee(p <= left, "p should start at or before left end of card"); - guarantee(p + obj->size() > left, "obj should end after left end of card"); + assert(p <= left, "p should start at or before left end of card"); + assert(p + obj->size() > left, "obj should end after left end of card"); return p; } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp index 0f3e711aa3372..1a44e8ed3fe0e 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp @@ -664,10 +664,9 @@ class ShenandoahCardCluster: public CHeapObj { // we use crossing maps to find where object's start, and use object sizes to walk individual objects. // The region must be parsable if ctx is null. // - // The last_relevant_card_index represents an upper bound on how far we look in the forward direction - // for the first object in the heap that intersects or follows this card. If there are no live objects - // found within the range of cards identified by [card_index, last_relevant_card_index], this function - // returns nullptr. + // The end_range_of_interest pointer argument represents an upper bound on how far we look in the forward direction + // for the first object in the heap that intersects or follows this card. If there are no live objects found at + // an address less than end_range_of_interest returns nullptr. // // Expects to be called for a card in a region affiliated with the old generation of the // generational heap, otherwise behavior is undefined. @@ -677,7 +676,7 @@ class ShenandoahCardCluster: public CHeapObj { // Note that crossing maps may be invalid following class unloading and before dead // or unloaded objects have been coalesced and filled. Coalesce and fill updates the crossing maps. HeapWord* first_object_start(size_t card_index, const ShenandoahMarkingContext* const ctx, - HeapWord* tams, const size_t last_relevant_card_index) const; + HeapWord* tams, HeapWord* end_range_of_interest) const; }; // ShenandoahScanRemembered is a concrete class representing the diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp index 37461e3b7ea3a..82d7938bb4cda 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp @@ -50,7 +50,7 @@ // degenerated execution, leading to dangling references. template void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t count, HeapWord* end_of_range, - ClosureType* cl, bool use_write_table, uint worker_id) { + ClosureType* cl, bool use_write_table, uint worker_id) { assert(ShenandoahHeap::heap()->old_generation()->is_parsable(), "Old generation regions must be parsable for remembered set scan"); // If old-gen evacuation is active, then MarkingContext for old-gen heap regions is valid. We use the MarkingContext @@ -67,10 +67,22 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou // clip at end_of_range (exclusive) HeapWord* end_addr = MIN2(end_of_range, (HeapWord*)start_addr + (count * ShenandoahCardCluster::CardsPerCluster * CardTable::card_size_in_words())); +#undef KELVIN_SCRUTINY +#ifdef KELVIN_SCRUTINY + log_info(gc)("end_addr (%zx) is MIN2(%zx, %zx)", p2i(end_addr), p2i(end_of_range), + p2i((HeapWord*)start_addr + (count * ShenandoahCardCluster::CardsPerCluster * CardTable::card_size_in_words()))); +#endif assert(start_addr < end_addr, "Empty region?"); const size_t whole_cards = (end_addr - start_addr + CardTable::card_size_in_words() - 1)/CardTable::card_size_in_words(); +#ifdef KELVIN_SCRUTINY + log_info(gc)("whole_cards: %zu span %zu words out of needed span %zu words", + whole_cards, whole_cards * CardTable::card_size_in_words(), end_addr - start_addr); +#endif const size_t end_card_index = start_card_index + whole_cards - 1; +#ifdef KELVIN_SCRUTINY + log_info(gc)("end_card_index: %zu, next_card_addr: %zx", end_card_index, p2i(_rs->addr_for_card_index(end_card_index + 1))); +#endif log_debug(gc, remset)("Worker %u: cluster = %zu count = %zu eor = " INTPTR_FORMAT " start_addr = " INTPTR_FORMAT " end_addr = " INTPTR_FORMAT " cards = %zu", worker_id, first_cluster, count, p2i(end_of_range), p2i(start_addr), p2i(end_addr), whole_cards); @@ -174,8 +186,10 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou // This is always the case for large object arrays, which are typically more // common. assert(ctx != nullptr || heap->old_generation()->is_parsable(), "Error"); - HeapWord* p = _scc->first_object_start(dirty_l, ctx, tams, dirty_r); - assert((p == nullptr) || (p < right), "No first object found is denoted by nullptr"); + HeapWord* p = _scc->first_object_start(dirty_l, ctx, tams, right); + assert((p == nullptr) || (p < right), "No first object found is denoted by nullptr, p: " + PTR_FORMAT ", right: " PTR_FORMAT ", end_addr: " PTR_FORMAT ", next card addr: " PTR_FORMAT, + p2i(p), p2i(right), p2i(end_addr), p2i(_rs->addr_for_card_index(dirty_r + 1))); if (p == nullptr) { // There are no live objects to be scanned in this dirty range. cur_index identifies first card in this // uninteresting dirty range. At top of next loop iteration, we will either end the looop From cee16f88dc61391eb68e6d02185a37de5a71b9a5 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Sun, 9 Nov 2025 09:06:29 -0700 Subject: [PATCH 30/32] fix multiple errors introduced by minor refactoring of API --- src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp | 1 + .../share/gc/shenandoah/shenandoahScanRemembered.cpp | 12 +++++++++--- .../share/gc/shenandoah/shenandoahScanRemembered.hpp | 2 ++ .../shenandoah/shenandoahScanRemembered.inline.hpp | 4 ++++ 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp index 6cbe44fef09fb..60ee92190b33c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp @@ -270,6 +270,7 @@ class ShenandoahHeap : public CollectedHeap { public: inline HeapWord* base() const { return _heap_region.start(); } + inline HeapWord* end() const { return _heap_region.end(); } inline size_t num_regions() const { return _num_regions; } inline bool is_heap_region_special() { return _heap_region_special; } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp index ecbb822abf6c9..3a99023eca42a 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp @@ -237,6 +237,7 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con HeapWord* tams, HeapWord* end_range_of_interest) const { HeapWord* left = _rs->addr_for_card_index(card_index); + assert(left < end_range_of_interest, "No meaningful work to do"); ShenandoahHeapRegion* region = ShenandoahHeap::heap()->heap_region_containing(left); #ifdef ASSERT @@ -249,9 +250,14 @@ HeapWord* ShenandoahCardCluster::first_object_start(const size_t card_index, con HeapWord* right = MIN2(region->top(), end_range_of_interest); HeapWord* end_of_search_next = MIN2(right, tams); - size_t 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--; + 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--; + } } assert(card_index <= last_relevant_card_index, "sanity: card_index: %zu, last_relevant: %zu, left: " PTR_FORMAT ", end_of_range: " PTR_FORMAT, card_index, last_relevant_card_index, p2i(left), p2i(end_range_of_interest)); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp index 1a44e8ed3fe0e..9a0d28d2cb754 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp @@ -353,6 +353,7 @@ class ShenandoahCardCluster: public CHeapObj { private: ShenandoahDirectCardMarkRememberedSet* _rs; + const HeapWord* _end_of_heap; public: static const size_t CardsPerCluster = 64; @@ -406,6 +407,7 @@ class ShenandoahCardCluster: public CHeapObj { ShenandoahCardCluster(ShenandoahDirectCardMarkRememberedSet* rs) { _rs = rs; + _end_of_heap = ShenandoahHeap::heap()->end(); _object_starts = NEW_C_HEAP_ARRAY(crossing_info, rs->total_cards() + 1, mtGC); // the +1 is to account for card table guarding entry for (size_t i = 0; i < rs->total_cards(); i++) { _object_starts[i].short_word = 0; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp index 82d7938bb4cda..31d63f30a0193 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp @@ -174,6 +174,10 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou // [left, right) is a maximal right-open interval of dirty cards HeapWord* left = _rs->addr_for_card_index(dirty_l); // inclusive HeapWord* right = _rs->addr_for_card_index(dirty_r + 1); // exclusive + if (end_addr <= left) { + // The range of addresses to be scanned is empty + continue; + } // Clip right to end_addr established above (still exclusive) right = MIN2(right, end_addr); assert(right <= region->top() && end_addr <= region->top(), "Busted bounds"); From 9f629a2a4f1a0ff76c76a376a8616f2105195ec3 Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Sun, 9 Nov 2025 16:11:18 +0000 Subject: [PATCH 31/32] Remove debug instrumentation --- .../shenandoah/shenandoahScanRemembered.inline.hpp | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp index 31d63f30a0193..e394daa68c0d8 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp @@ -67,22 +67,10 @@ void ShenandoahScanRemembered::process_clusters(size_t first_cluster, size_t cou // clip at end_of_range (exclusive) HeapWord* end_addr = MIN2(end_of_range, (HeapWord*)start_addr + (count * ShenandoahCardCluster::CardsPerCluster * CardTable::card_size_in_words())); -#undef KELVIN_SCRUTINY -#ifdef KELVIN_SCRUTINY - log_info(gc)("end_addr (%zx) is MIN2(%zx, %zx)", p2i(end_addr), p2i(end_of_range), - p2i((HeapWord*)start_addr + (count * ShenandoahCardCluster::CardsPerCluster * CardTable::card_size_in_words()))); -#endif assert(start_addr < end_addr, "Empty region?"); const size_t whole_cards = (end_addr - start_addr + CardTable::card_size_in_words() - 1)/CardTable::card_size_in_words(); -#ifdef KELVIN_SCRUTINY - log_info(gc)("whole_cards: %zu span %zu words out of needed span %zu words", - whole_cards, whole_cards * CardTable::card_size_in_words(), end_addr - start_addr); -#endif const size_t end_card_index = start_card_index + whole_cards - 1; -#ifdef KELVIN_SCRUTINY - log_info(gc)("end_card_index: %zu, next_card_addr: %zx", end_card_index, p2i(_rs->addr_for_card_index(end_card_index + 1))); -#endif log_debug(gc, remset)("Worker %u: cluster = %zu count = %zu eor = " INTPTR_FORMAT " start_addr = " INTPTR_FORMAT " end_addr = " INTPTR_FORMAT " cards = %zu", worker_id, first_cluster, count, p2i(end_of_range), p2i(start_addr), p2i(end_addr), whole_cards); From 2dc7e98dffcc6e2f206fc0f4b9ea67f07b1e889c Mon Sep 17 00:00:00 2001 From: Kelvin Nilsen Date: Mon, 10 Nov 2025 23:40:15 +0000 Subject: [PATCH 32/32] Add two comments --- src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp index 349148dd41f94..73bf3ecbeea9b 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp @@ -126,11 +126,13 @@ class ShenandoahMarkBitMap { template inline idx_t get_prev_bit_impl(idx_t l_index, idx_t r_index) const; + // Search for the first marked address in the range [l_index, r_index), or r_index if none found. inline idx_t get_next_one_offset(idx_t l_index, idx_t r_index) const; // 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; + // Clear the strong and weak mark bits for all index positions >= l_index and < r_index. void clear_large_range(idx_t beg, idx_t end); // Verify bit is less than size().