Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
eec0844
Fix incorrect loop, tweak asserts.
ysramakrishna Jun 6, 2025
f28571d
Enable #undef's code for testing
ysramakrishna Jun 6, 2025
893f5e8
Refine fix further
ysramakrishna Jun 6, 2025
256944a
More cleanups.
ysramakrishna Jun 6, 2025
051e574
More testing; including in product builds. Product mode guarantees will
ysramakrishna Jun 10, 2025
6c607af
Merge branch 'master' into block_start
ysramakrishna Jul 27, 2025
0b4eac8
Replace calls to inlined method codes in asserts with class to methods.
ysramakrishna Aug 13, 2025
f304980
Merge branch 'master' into block_start
ysramakrishna Aug 13, 2025
212aed4
Fix call to static method in assert.
ysramakrishna Aug 13, 2025
b8fff8d
May not be checked in; slightly stricter verification of blocks in bl…
ysramakrishna Aug 19, 2025
e2b61ed
first_object_start() doesn't yet do the right thing. Caller should
ysramakrishna Aug 20, 2025
c1356ef
looking back over a (shenmarking)bitmap. WIP, has (plenty of) bugs.
ysramakrishna Aug 20, 2025
63fde7e
Stash before proceeding on vacation. This branch has bugs that need to
ysramakrishna Sep 6, 2025
69452aa
Some early efforts on understanding problems with block_start improve…
kdnilsen Sep 10, 2025
7b48ebf
some progress on get_last_marked_object()
kdnilsen Sep 15, 2025
ca728de
Add special handling above TAMS
kdnilsen Sep 16, 2025
faaf779
Revert extraneous changes
kdnilsen Sep 16, 2025
4f1057e
Add handling for first_object_start() past end of range
kdnilsen Sep 17, 2025
490638f
Merge remote-tracking branch 'jdk/master' into finish-block-start
kdnilsen Sep 17, 2025
84ad6b6
Remove troublesome assert that assumes lock is held
kdnilsen Sep 17, 2025
0583a04
add explicit typecast to avoid compiler warning message
kdnilsen Sep 17, 2025
7578484
disable for debug build, alphabetic order for includes
kdnilsen Sep 17, 2025
9c87c2f
fix white space
kdnilsen Sep 17, 2025
349818d
Fix order of include files
kdnilsen Sep 18, 2025
7697942
cleanup code for review
kdnilsen Sep 26, 2025
668e861
fix idiosyncratic formatting
kdnilsen Oct 3, 2025
80198ab
Fixup handling of weakly marked objects in remembered set
kdnilsen Oct 20, 2025
e16ea23
fix bugs in implementation of weakly referenced object handling
kdnilsen Nov 1, 2025
d341522
Merge remote-tracking branch 'jdk/master' into finish-block-start
kdnilsen Nov 1, 2025
643cdfd
Revert "Fixup handling of weakly marked objects in remembered set"
kdnilsen Nov 5, 2025
637c177
fix up comments and simplify API for ShenandoahScanRemembered::first_…
kdnilsen Nov 7, 2025
0e2120b
consider last_relevant_card in determining right-most address
kdnilsen Nov 7, 2025
29f5d42
Refinements and debugging
kdnilsen Nov 8, 2025
cee16f8
fix multiple errors introduced by minor refactoring of API
kdnilsen Nov 9, 2025
9f629a2
Remove debug instrumentation
kdnilsen Nov 9, 2025
2dc7e98
Add two comments
kdnilsen Nov 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
20 changes: 20 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,26 @@ bool ShenandoahMarkBitMap::is_bitmap_clear_range(const HeapWord* start, const He
return (result == end);
}

HeapWord* ShenandoahMarkBitMap::get_prev_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
Comment on lines +62 to +70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


// 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_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);
}

HeapWord* ShenandoahMarkBitMap::get_next_marked_addr(const HeapWord* addr,
const HeapWord* limit) const {
Expand Down
24 changes: 19 additions & 5 deletions src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,21 @@ class ShenandoahMarkBitMap {
template<bm_word_t flip, bool aligned_right>
inline idx_t get_next_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;
// 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<bm_word_t flip, bool aligned_left>
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document analogous to line 131.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


void clear_large_range (idx_t beg, idx_t end);
// Search for last one in the range [l_index, r_index). Return r_index if not found.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

l_index <-> beg
r_index <-> end

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment here as well.


// Verify bit is less than size().
void verify_index(idx_t bit) const NOT_DEBUG_RETURN;
Expand Down Expand Up @@ -162,12 +174,14 @@ 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 addr+1 if none found.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
void clear_range_within_word (idx_t beg, idx_t end);
void clear_range (idx_t beg, idx_t end);
Expand Down
90 changes: 90 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "gc/shenandoah/shenandoahMarkBitMap.hpp"

#include "runtime/atomicAccess.hpp"
#include "utilities/count_leading_zeros.hpp"
#include "utilities/count_trailing_zeros.hpp"

inline size_t ShenandoahMarkBitMap::address_to_index(const HeapWord* addr) const {
Expand Down Expand Up @@ -169,10 +170,99 @@ inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_bit_impl(idx_t
return r_index;
}

template<ShenandoahMarkBitMap::bm_word_t flip, bool aligned_left>
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");

// 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 the high-order bits (representing objects that come after r_index)
idx_t index = to_words_align_down(r_index);
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.

// 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_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) {
// 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);
// 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) {
// 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 r_index+2.
return r_index + 2;
}
}
else {
return r_index + 2;
}
}

inline ShenandoahMarkBitMap::idx_t ShenandoahMarkBitMap::get_next_one_offset(idx_t l_offset, idx_t r_offset) const {
return get_next_bit_impl<find_ones_flip, false>(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<find_ones_flip, false>(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
Expand Down
4 changes: 4 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,12 @@ class ShenandoahMarkingContext : public CHeapObj<mtGC> {
inline bool is_marked_or_old(oop obj) const;
inline bool is_marked_strong_or_old(oop obj) const;

// 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;

// 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* start) const;

inline bool allocated_after_mark_start(const oop obj) const;
inline bool allocated_after_mark_start(const HeapWord* addr) const;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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_prev_marked_addr(const HeapWord* limit, const HeapWord* start) const {
return _mark_bit_map.get_prev_marked_addr(limit, start);
}

inline bool ShenandoahMarkingContext::allocated_after_mark_start(oop obj) const {
const HeapWord* addr = cast_from_oop<HeapWord*>(obj);
return allocated_after_mark_start(addr);
Expand Down
Loading