Skip to content

8351091: Shenandoah: global marking context completeness is not accurately maintained #23886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 18 commits into from
Closed

8351091: Shenandoah: global marking context completeness is not accurately maintained #23886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4830bc9
JDK-8351091: initial works
pengxiaolong Mar 4, 2025
5414448
promotion in place and reference processor should be aware of heap ge…
pengxiaolong Mar 4, 2025
dabcf64
Fix improper use of heap->complete_marking_context()
pengxiaolong Mar 4, 2025
d159f6f
Remove ShenandoahHeap::complete_marking_context()
pengxiaolong Mar 4, 2025
d3f4c91
If GC generation is young and referent is in old, make should_drop re…
pengxiaolong Mar 4, 2025
6ded66a
touch up
pengxiaolong Mar 4, 2025
e4f9d8f
Revert the change in ShenandoahHeap::generation_for
pengxiaolong Mar 4, 2025
01c6ea6
Revert unnecessary changes in ShenandoahReferenceProcessor
pengxiaolong Mar 4, 2025
465deae
Always use active_generation()->complete_marking_context() during ref…
pengxiaolong Mar 5, 2025
aa4a83f
Address review comments
pengxiaolong Mar 5, 2025
c78f66e
Remove obsolete code comments
pengxiaolong Mar 5, 2025
2004973
complete_marking_context should guarantee mark is complete
pengxiaolong Mar 5, 2025
d893918
Revert "complete_marking_context should guarantee mark is complete"
pengxiaolong Mar 6, 2025
952f7ea
Remove ShenandoahHeap::complete_marking_context(ShenandoahHeapRegion*…
pengxiaolong Mar 6, 2025
17bcb35
Address review comments
pengxiaolong Mar 7, 2025
c95c0f1
Merge branch 'master' into JDK-8351091
pengxiaolong Mar 31, 2025
7c73e12
Can't verify marked object with complete marking after full GC
pengxiaolong Mar 31, 2025
d4af962
Address PR comments
pengxiaolong Apr 4, 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
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/shenandoah/heuristics/shenandoahGenerationalHeuristics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ void ShenandoahGenerationalHeuristics::choose_collection_set(ShenandoahCollectio
// Reclaim humongous regions here, and count them as the immediate garbage
#ifdef ASSERT
bool reg_live = region->has_live();
bool bm_live = heap->complete_marking_context()->is_marked(cast_to_oop(region->bottom()));
bool bm_live = heap->active_generation()->complete_marking_context()->is_marked(cast_to_oop(region->bottom()));
assert(reg_live == bm_live,
"Humongous liveness and marks should agree. Region live: %s; Bitmap live: %s; Region Live Words: %zu",
BOOL_TO_STR(reg_live), BOOL_TO_STR(bm_live), region->get_live_data_words());
Expand Down
4 changes: 1 addition & 3 deletions src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,6 @@ void ShenandoahHeuristics::choose_collection_set(ShenandoahCollectionSet* collec
size_t free = 0;
size_t free_regions = 0;

ShenandoahMarkingContext* const ctx = heap->complete_marking_context();

for (size_t i = 0; i < num_regions; i++) {
ShenandoahHeapRegion* region = heap->get_region(i);

Expand All @@ -122,7 +120,7 @@ void ShenandoahHeuristics::choose_collection_set(ShenandoahCollectionSet* collec
// Reclaim humongous regions here, and count them as the immediate garbage
#ifdef ASSERT
bool reg_live = region->has_live();
bool bm_live = ctx->is_marked(cast_to_oop(region->bottom()));
bool bm_live = heap->gc_generation()->complete_marking_context()->is_marked(cast_to_oop(region->bottom()));
assert(reg_live == bm_live,
"Humongous liveness and marks should agree. Region live: %s; Bitmap live: %s; Region Live Words: %zu",
BOOL_TO_STR(reg_live), BOOL_TO_STR(bm_live), region->get_live_data_words());
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,7 +1030,7 @@ HeapWord* ShenandoahFreeSet::try_allocate_in(ShenandoahHeapRegion* r, Shenandoah
_heap->generation_for(r->affiliation())->increment_affiliated_region_count();

#ifdef ASSERT
ShenandoahMarkingContext* const ctx = _heap->complete_marking_context();
ShenandoahMarkingContext* const ctx = _heap->marking_context();
Copy link
Member

@ysramakrishna ysramakrishna Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why not this instead?

 ShenandoahMarkingContext* const ctx = _heap->marking_context(r);

Copy link
Author

@pengxiaolong pengxiaolong Mar 5, 2025

Choose a reason for hiding this comment

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

Technically there is only one global marking context for Shenandoah, even in generational mode, passing the region to marking_context doesn't make any difference.

But in the method complete_marking_context(r), it checks if the affiliated generation has complete marking, it is a more convenient version of complete_marking_context(affiliation).

Copy link
Member

@ysramakrishna ysramakrishna Mar 6, 2025

Choose a reason for hiding this comment

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

OK, yes, that makes sense. Why not then use both ShenandoahHeap::[complete_]marking_context() as synonyms for ShehandoahHeap::active_generation()->[complete_]marking_context(). See other related comments in this review round.

Copy link
Author

@pengxiaolong pengxiaolong Mar 6, 2025

Choose a reason for hiding this comment

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

I feel using henandoahHeap::complete_marking_context() as synonyms for ShehandoahHeap::active_generation()->[complete_]marking_context() may cause more confusion, just read from the name it seems that it indicates the marking is complete for the whole heap, not just the active generation.

Copy link
Member

@ysramakrishna ysramakrishna Apr 3, 2025

Choose a reason for hiding this comment

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

ok, that makes sense.

assert(ctx->top_at_mark_start(r) == r->bottom(), "Newly established allocation region starts with TAMS equal to bottom");
assert(ctx->is_bitmap_range_within_region_clear(ctx->top_bitmap(r), r->end()), "Bitmap above top_bitmap() must be clear");
#endif
Expand Down
16 changes: 8 additions & 8 deletions src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,8 @@ class ShenandoahPrepareForCompactionObjectClosure : public ObjectClosure {

void do_object(oop p) {
assert(_from_region != nullptr, "must set before work");
assert(_heap->complete_marking_context()->is_marked(p), "must be marked");
assert(!_heap->complete_marking_context()->allocated_after_mark_start(p), "must be truly marked");
assert(_heap->gc_generation()->complete_marking_context()->is_marked(p), "must be marked");
assert(!_heap->gc_generation()->complete_marking_context()->allocated_after_mark_start(p), "must be truly marked");

size_t obj_size = p->size();
if (_compact_point + obj_size > _to_region->end()) {
Expand Down Expand Up @@ -551,7 +551,7 @@ class ShenandoahTrashImmediateGarbageClosure: public ShenandoahHeapRegionClosure
public:
ShenandoahTrashImmediateGarbageClosure() :
_heap(ShenandoahHeap::heap()),
_ctx(ShenandoahHeap::heap()->complete_marking_context()) {}
_ctx(ShenandoahHeap::heap()->global_generation()->complete_marking_context()) {}

void heap_region_do(ShenandoahHeapRegion* r) override {
if (r->is_humongous_start()) {
Expand Down Expand Up @@ -775,7 +775,7 @@ class ShenandoahAdjustPointersClosure : public MetadataVisitingOopIterateClosure
public:
ShenandoahAdjustPointersClosure() :
_heap(ShenandoahHeap::heap()),
_ctx(ShenandoahHeap::heap()->complete_marking_context()) {}
_ctx(ShenandoahHeap::heap()->gc_generation()->complete_marking_context()) {}

void do_oop(oop* p) { do_oop_work(p); }
void do_oop(narrowOop* p) { do_oop_work(p); }
Expand All @@ -793,7 +793,7 @@ class ShenandoahAdjustPointersObjectClosure : public ObjectClosure {
_heap(ShenandoahHeap::heap()) {
}
void do_object(oop p) {
assert(_heap->complete_marking_context()->is_marked(p), "must be marked");
assert(_heap->gc_generation()->complete_marking_context()->is_marked(p), "must be marked");
p->oop_iterate(&_cl);
}
};
Expand Down Expand Up @@ -877,7 +877,7 @@ class ShenandoahCompactObjectsClosure : public ObjectClosure {
_heap(ShenandoahHeap::heap()), _worker_id(worker_id) {}

void do_object(oop p) {
assert(_heap->complete_marking_context()->is_marked(p), "must be marked");
assert(_heap->gc_generation()->complete_marking_context()->is_marked(p), "must be marked");
size_t size = p->size();
if (FullGCForwarding::is_forwarded(p)) {
HeapWord* compact_from = cast_from_oop<HeapWord*>(p);
Expand Down Expand Up @@ -950,7 +950,7 @@ class ShenandoahPostCompactClosure : public ShenandoahHeapRegionClosure {
// NOTE: See blurb at ShenandoahMCResetCompleteBitmapTask on why we need to skip
// pinned regions.
if (!r->is_pinned()) {
_heap->complete_marking_context()->reset_top_at_mark_start(r);
_heap->gc_generation()->complete_marking_context()->reset_top_at_mark_start(r);
}

size_t live = r->used();
Expand Down Expand Up @@ -1091,7 +1091,7 @@ class ShenandoahMCResetCompleteBitmapTask : public WorkerTask {
ShenandoahParallelWorkerSession worker_session(worker_id);
ShenandoahHeapRegion* region = _regions.next();
ShenandoahHeap* heap = ShenandoahHeap::heap();
ShenandoahMarkingContext* const ctx = heap->complete_marking_context();
ShenandoahMarkingContext* const ctx = heap->gc_generation()->complete_marking_context();
while (region != nullptr) {
if (heap->is_bitmap_slice_committed(region) && !region->is_pinned() && region->has_live()) {
ctx->clear_bitmap(region);
Expand Down
4 changes: 0 additions & 4 deletions src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,10 +775,6 @@ bool ShenandoahGeneration::is_bitmap_clear() {
return true;
}

bool ShenandoahGeneration::is_mark_complete() {
return _is_marking_complete.is_set();
}

void ShenandoahGeneration::set_mark_complete() {
_is_marking_complete.set();
}
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/shenandoah/shenandoahGeneration.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ class ShenandoahGeneration : public CHeapObj<mtGC>, public ShenandoahSpaceInfo {
bool is_bitmap_clear();

// We need to track the status of marking for different generations.
bool is_mark_complete();
bool is_mark_complete() { return _is_marking_complete.is_set(); }
virtual void set_mark_complete();
virtual void set_mark_incomplete();
Comment on lines 205 to 206
Copy link
Member

@ysramakrishna ysramakrishna Mar 4, 2025

Choose a reason for hiding this comment

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

Why are these declared virtual?

Copy link
Member

@ysramakrishna ysramakrishna Mar 4, 2025

Choose a reason for hiding this comment

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

OK, I see that ShenandoahGlobalGeneration forces the state of ShenandoahOdGeneration and ShenandoahYoungGeneration, but is that our intention? I am seeing (see comment elsewhere) that we are always either using global generation's marking context explicitly, or using a region to index into the appropriate containing generation's marking context. If so, can we dispense with the forcing of global context's state into the contexts for the two generations?

Copy link
Author

@pengxiaolong pengxiaolong Mar 6, 2025

Choose a reason for hiding this comment

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

If so, can we dispense with the forcing of global context's state into the contexts for the two generations?

I think we can do that if we deprecated the classical mode and only support generational Shenandoah, in classical mode, there is only global generation.

Copy link
Member

@ysramakrishna ysramakrishna Mar 6, 2025

Choose a reason for hiding this comment

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

I am not sure I follow. In the legacy (non-generational mode) we shouldn't care about the marking state of the old and young generations, just that of the GlobalGeneration. In the generational case, we explicitly track the marking state of the old and young generations explicitly. It sounds to me as if forcing the Old and Young marking states to the state of that of the GlobalGeneration must be exactly for the case where we are using Generational Shenandoah, and we are doing a Global collection? Indeed:

void ShenandoahGlobalGeneration::set_mark_complete() {
  ShenandoahGeneration::set_mark_complete();
  if (ShenandoahHeap::heap()->mode()->is_generational()) {
    ShenandoahGenerationalHeap* heap = ShenandoahGenerationalHeap::heap();
    heap->young_generation()->set_mark_complete();
    heap->old_generation()->set_mark_complete();
  }
}

I am saying that each of Old, Young, and Global generations maintain their own mark completion state and use that to determine what they pass back in response to complete_marking_context(). This completely localizes all state rather than unnecessarily and confusingly coupling the states of these generations.

So, you remove the part in the if branch in the code above, which reduces to the default (or rather only) implementation in the base class, not requiring the over-ride of the Global generation's method for the generational case.

void ShenandoahGeneration::set_mark_complete() {
  _is_marking_complete.set();
}

It is possible that I am still missing the actual structure here that requires the override for GlobalGeneration for the generational case.

Copy link
Author

@pengxiaolong pengxiaolong Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I misunderstood your original proposal, I thought you meant to suggest to remove the flag from ShenandoahGlobalGeneration, instead the set_mark_complete/is_mark_complete will more like view/delegation layer like:

void ShenandoahGlobalGeneration::set_mark_complete() {
    ShenandoahGenerationalHeap* heap = ShenandoahGenerationalHeap::heap();
    heap->young_generation()->set_mark_complete();
    heap->old_generation()->set_mark_complete();
}

bool ShenandoahGlobalGeneration::is_mark_complete() {
    ShenandoahGenerationalHeap* heap = ShenandoahGenerationalHeap::heap();
    return heap->young_generation()->is_mark_complete() && heap->old_generation()->is_mark_complete();
}

Copy link
Author

@pengxiaolong pengxiaolong Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You proposal will make the impl of the set_mark_complete/is_mark_complete of ShenandoahGeneration cleaner, but the thing is it will change current design and behavior, we may have to update the code where there methods is called, e.g. when we call set_mark_complete of gc_generation/active_generation, if it is global generation, we may have to explicitly call the same methods of ShenandoahYoungGeneration and ShenandoahOldGeneration to fan out the status.

How about I follow up it in a separate task and update the implementation if necessary? I want to limit the changes involved in this PR, and only fix the bug.

Copy link
Contributor

@earthling-amzn earthling-amzn Mar 7, 2025

Choose a reason for hiding this comment

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

The young and old generations are only instantiated in the generational mode, so using them without checking the mode will result in SEGV in non-generational modes.

Global collections have a lot of overlap with old collections. I think what Ramki is saying, is that if we change all the code that makes assertions about the completion status of young/old marking to use the active_generation field instead, then we wouldn't need to update the completion status of young/old during a global collection. The difficulty here is that we need assurances that the old generation mark bitmap is valid in collections subsequent to a global collection. So, I don't think we can rely on completion status of active_generation when it was global, in following collections where it may now refer to young or old.

Copy link
Member

@ysramakrishna ysramakrishna Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Yes, that makes sense to me, thanks William.

It would then be the case for the global generation that if is_mark_complete() then in the generational case that's also the case for both of its constituent generations. May be we can assert that when we fetch that at line 204 (and find it's true) for the case where we make the method is_mark_complete() also virtual :-) ?

May be I am being paranoid, but the assert would make me feel confident that the state maintenance isn't going awry/askew.


Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,8 @@ void ShenandoahGenerationalEvacuationTask::maybe_promote_region(ShenandoahHeapRe
// We identify the entirety of the region as DIRTY to force the next remembered set scan to identify the "interesting pointers"
// contained herein.
void ShenandoahGenerationalEvacuationTask::promote_in_place(ShenandoahHeapRegion* region) {
ShenandoahMarkingContext* const marking_context = _heap->complete_marking_context();
assert(!_heap->gc_generation()->is_old(), "Sanity check");
ShenandoahMarkingContext* const marking_context = _heap->young_generation()->complete_marking_context();
Copy link
Member

@ysramakrishna ysramakrishna Mar 6, 2025

Choose a reason for hiding this comment

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

For clarity, you might assert the following before line 172:

    assert(gc_generation() == _heap->young_generation(), "Sanity check");

Even though it might seem somewhat tautological.

Copy link
Author

@pengxiaolong pengxiaolong Mar 6, 2025

Choose a reason for hiding this comment

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

Thanks, I'll add it.

Copy link
Author

@pengxiaolong pengxiaolong Mar 6, 2025

Choose a reason for hiding this comment

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

Question: Does Shenandoah promote region in global cycles? the gc_generation might be global if so.

Copy link
Member

@ysramakrishna ysramakrishna Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. I don't see any reason promotions should be verboten in global cycles. cc @earthling-amzn ?

If that is indeed the case, a clean separation and maintenance of completeness of marking for global generation, and use of _heap->gc_generation() would make sense to me.

Copy link
Author

@pengxiaolong pengxiaolong Mar 7, 2025

Choose a reason for hiding this comment

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

Thanks for the confirmation, I added assert as below since it gc_generation could be global :

assert(!_heap->gc_generation()->is_old(), "Sanity check");

Copy link
Member

@ysramakrishna ysramakrishna Mar 7, 2025

Choose a reason for hiding this comment

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

The assert may be fine, but the treatment of completeness of the marking context seems very brittle to me and apt to cause problems in the future. I would prefer a cleaner separation of these. May be we can sync up separately to discuss this along with @earthling-amzn .

Copy link
Contributor

@earthling-amzn earthling-amzn Mar 7, 2025

Choose a reason for hiding this comment

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

Yes, regions may be promoted during a global cycle. Completing the mark for a global cycle also completes the mark for the young and old generations.

HeapWord* const tams = marking_context->top_at_mark_start(region);

{
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/gc/shenandoah/shenandoahGenerationalFullGC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,8 @@ void ShenandoahPrepareForGenerationalCompactionObjectClosure::do_object(oop p) {
assert(_from_region != nullptr, "must set before work");
assert((_from_region->bottom() <= cast_from_oop<HeapWord*>(p)) && (cast_from_oop<HeapWord*>(p) < _from_region->top()),
"Object must reside in _from_region");
assert(_heap->complete_marking_context()->is_marked(p), "must be marked");
assert(!_heap->complete_marking_context()->allocated_after_mark_start(p), "must be truly marked");
assert(_heap->global_generation()->complete_marking_context()->is_marked(p), "must be marked");
assert(!_heap->global_generation()->complete_marking_context()->allocated_after_mark_start(p), "must be truly marked");

size_t obj_size = p->size();
uint from_region_age = _from_region->age();
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,8 +424,6 @@ jint ShenandoahHeap::initialize() {
_affiliations[i] = ShenandoahAffiliation::FREE;
}

// Initialize to complete
_marking_context->mark_complete();
size_t young_cset_regions, old_cset_regions;

// We are initializing free set. We ignore cset region tallies.
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ class ShenandoahHeap : public CollectedHeap {
ShenandoahLiveData** _liveness_cache;

public:
inline ShenandoahMarkingContext* complete_marking_context() const;
// Return the marking context regardless of the completeness status.
inline ShenandoahMarkingContext* marking_context() const;

template<class T>
Expand Down
5 changes: 0 additions & 5 deletions src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,11 +643,6 @@ inline ShenandoahHeapRegion* ShenandoahHeap::get_region(size_t region_idx) const
}
}

inline ShenandoahMarkingContext* ShenandoahHeap::complete_marking_context() const {
assert (_marking_context->is_complete()," sanity");
return _marking_context;
}

inline ShenandoahMarkingContext* ShenandoahHeap::marking_context() const {
return _marking_context;
}
Expand Down
4 changes: 1 addition & 3 deletions src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,8 +855,8 @@ void ShenandoahHeapRegion::set_affiliation(ShenandoahAffiliation new_affiliation
ShenandoahHeap* heap = ShenandoahHeap::heap();

ShenandoahAffiliation region_affiliation = heap->region_affiliation(this);
ShenandoahMarkingContext* const ctx = heap->marking_context();
{
ShenandoahMarkingContext* const ctx = heap->complete_marking_context();
log_debug(gc)("Setting affiliation of Region %zu from %s to %s, top: " PTR_FORMAT ", TAMS: " PTR_FORMAT
", watermark: " PTR_FORMAT ", top_bitmap: " PTR_FORMAT,
index(), shenandoah_affiliation_name(region_affiliation), shenandoah_affiliation_name(new_affiliation),
Expand All @@ -865,8 +865,6 @@ void ShenandoahHeapRegion::set_affiliation(ShenandoahAffiliation new_affiliation

#ifdef ASSERT
{
// During full gc, heap->complete_marking_context() is not valid, may equal nullptr.
ShenandoahMarkingContext* const ctx = heap->complete_marking_context();
size_t idx = this->index();
HeapWord* top_bitmap = ctx->top_bitmap(this);

Expand Down
14 changes: 2 additions & 12 deletions src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include "gc/shenandoah/shenandoahHeap.inline.hpp"
#include "gc/shenandoah/shenandoahMarkingContext.hpp"

#include "shenandoahGlobalGeneration.hpp"

ShenandoahMarkingContext::ShenandoahMarkingContext(MemRegion heap_region, MemRegion bitmap_region, size_t num_regions) :
_mark_bit_map(heap_region, bitmap_region),
_top_bitmaps(NEW_C_HEAP_ARRAY(HeapWord*, num_regions, mtGC)),
Expand Down Expand Up @@ -96,15 +98,3 @@ void ShenandoahMarkingContext::clear_bitmap(ShenandoahHeapRegion* r) {
assert(is_bitmap_range_within_region_clear(bottom, r->end()),
"Region %zu should have no marks in bitmap", r->index());
}

bool ShenandoahMarkingContext::is_complete() {
return _is_complete.is_set();
}

void ShenandoahMarkingContext::mark_complete() {
_is_complete.set();
}

void ShenandoahMarkingContext::mark_incomplete() {
_is_complete.unset();
}
6 changes: 0 additions & 6 deletions src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ class ShenandoahMarkingContext : public CHeapObj<mtGC> {
HeapWord** const _top_at_mark_starts_base;
HeapWord** const _top_at_mark_starts;

ShenandoahSharedFlag _is_complete;

public:
ShenandoahMarkingContext(MemRegion heap_region, MemRegion bitmap_region, size_t num_regions);

Expand Down Expand Up @@ -86,10 +84,6 @@ class ShenandoahMarkingContext : public CHeapObj<mtGC> {

bool is_bitmap_clear() const;
bool is_bitmap_range_within_region_clear(const HeapWord* start, const HeapWord* end) const;

bool is_complete();
void mark_complete();
void mark_incomplete();
};

#endif // SHARE_GC_SHENANDOAH_SHENANDOAHMARKINGCONTEXT_HPP
9 changes: 5 additions & 4 deletions src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,12 +328,13 @@ bool ShenandoahReferenceProcessor::should_drop(oop reference, ReferenceType type
return true;
}

ShenandoahHeap* heap = ShenandoahHeap::heap();
// Check if the referent is still alive, in which case we should
// drop the reference.
if (type == REF_PHANTOM) {
return ShenandoahHeap::heap()->complete_marking_context()->is_marked(raw_referent);
return heap->active_generation()->complete_marking_context()->is_marked(raw_referent);
} else {
return ShenandoahHeap::heap()->complete_marking_context()->is_marked_strong(raw_referent);
return heap->active_generation()->complete_marking_context()->is_marked_strong(raw_referent);
}
}

Expand All @@ -345,7 +346,7 @@ void ShenandoahReferenceProcessor::make_inactive(oop reference, ReferenceType ty
// next field. An application can't call FinalReference.enqueue(), so there is
// no race to worry about when setting the next field.
assert(reference_next<T>(reference) == nullptr, "Already inactive");
assert(ShenandoahHeap::heap()->marking_context()->is_marked(reference_referent_raw<T>(reference)), "only make inactive final refs with alive referents");
assert(ShenandoahHeap::heap()->active_generation()->complete_marking_context()->is_marked(reference_referent_raw<T>(reference)), "only make inactive final refs with alive referents");
reference_set_next(reference, reference);
} else {
// Clear referent
Expand Down Expand Up @@ -435,7 +436,7 @@ oop ShenandoahReferenceProcessor::drop(oop reference, ReferenceType type) {
HeapWord* raw_referent = reference_referent_raw<T>(reference);

#ifdef ASSERT
assert(raw_referent == nullptr || ShenandoahHeap::heap()->marking_context()->is_marked(raw_referent),
assert(raw_referent == nullptr || ShenandoahHeap::heap()->active_generation()->complete_marking_context()->is_marked(raw_referent),
"only drop references with alive referents");
#endif

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/shenandoah/shenandoahUnload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class ShenandoahIsUnloadingOopClosure : public OopClosure {

public:
ShenandoahIsUnloadingOopClosure() :
_marking_context(ShenandoahHeap::heap()->complete_marking_context()),
_marking_context(ShenandoahHeap::heap()->global_generation()->complete_marking_context()),
_is_unloading(false) {
}

Expand Down
20 changes: 11 additions & 9 deletions src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp
View file
Open in desktop
Copy link
Member

@ysramakrishna ysramakrishna Mar 6, 2025

Choose a reason for hiding this comment

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

See previous suggestion/comments on SH::[complete_]marking_context() as delegating to that method of its gc_generation().

What you have here sounds fine too, but a uniform usage of either keeping SH::[complete_]marking_context() or not at all makes more sense to me, and seems cleaner to me.

Original file line number Diff line number Diff line change
Expand Up @@ -268,12 +268,12 @@ class ShenandoahVerifyOopClosure : public BasicOopIterateClosure {
"Must be marked in incomplete bitmap");
break;
case ShenandoahVerifier::_verify_marked_complete:
check(ShenandoahAsserts::_safe_all, obj, _heap->complete_marking_context()->is_marked(obj),
check(ShenandoahAsserts::_safe_all, obj, _heap->gc_generation()->complete_marking_context()->is_marked(obj),
"Must be marked in complete bitmap");
break;
case ShenandoahVerifier::_verify_marked_complete_except_references:
case ShenandoahVerifier::_verify_marked_complete_satb_empty:
check(ShenandoahAsserts::_safe_all, obj, _heap->complete_marking_context()->is_marked(obj),
check(ShenandoahAsserts::_safe_all, obj, _heap->gc_generation()->complete_marking_context()->is_marked(obj),
"Must be marked in complete bitmap, except j.l.r.Reference referents");
break;
default:
Expand Down Expand Up @@ -701,15 +701,15 @@ class ShenandoahVerifierMarkedRegionTask : public WorkerTask {
virtual void work_humongous(ShenandoahHeapRegion *r, ShenandoahVerifierStack& stack, ShenandoahVerifyOopClosure& cl) {
size_t processed = 0;
HeapWord* obj = r->bottom();
if (_heap->complete_marking_context()->is_marked(cast_to_oop(obj))) {
if (_heap->gc_generation()->complete_marking_context()->is_marked(cast_to_oop(obj))) {
verify_and_follow(obj, stack, cl, &processed);
}
Atomic::add(&_processed, processed, memory_order_relaxed);
}

virtual void work_regular(ShenandoahHeapRegion *r, ShenandoahVerifierStack &stack, ShenandoahVerifyOopClosure &cl) {
size_t processed = 0;
ShenandoahMarkingContext* ctx = _heap->complete_marking_context();
ShenandoahMarkingContext* ctx = _heap->gc_generation()->complete_marking_context();
HeapWord* tams = ctx->top_at_mark_start(r);

// Bitmaps, before TAMS
Expand Down Expand Up @@ -788,9 +788,11 @@ class VerifyThreadGCState : public ThreadClosure {

void ShenandoahVerifier::verify_at_safepoint(const char* label,
VerifyRememberedSet remembered,
VerifyForwarded forwarded, VerifyMarked marked,
VerifyForwarded forwarded,
VerifyMarked marked,
VerifyCollectionSet cset,
VerifyLiveness liveness, VerifyRegions regions,
VerifyLiveness liveness,
VerifyRegions regions,
VerifySize sizeness,
VerifyGCState gcstate) {
guarantee(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "only when nothing else happens");
Expand Down Expand Up @@ -989,7 +991,7 @@ void ShenandoahVerifier::verify_at_safepoint(const char* label,
(marked == _verify_marked_complete ||
marked == _verify_marked_complete_except_references ||
marked == _verify_marked_complete_satb_empty)) {
guarantee(_heap->marking_context()->is_complete(), "Marking context should be complete");
guarantee(_heap->gc_generation()->is_mark_complete(), "Marking context should be complete");
ShenandoahVerifierMarkedRegionTask task(_verification_bit_map, ld, label, options);
_heap->workers()->run_task(&task);
count_marked = task.processed();
Expand Down Expand Up @@ -1186,7 +1188,7 @@ void ShenandoahVerifier::verify_after_fullgc() {
"After Full GC",
_verify_remembered_after_full_gc, // verify read-write remembered set
_verify_forwarded_none, // all objects are non-forwarded
_verify_marked_complete, // all objects are marked in complete bitmap
_verify_marked_incomplete, // all objects are marked in incomplete bitmap
_verify_cset_none, // no cset references
_verify_liveness_disable, // no reliable liveness data anymore
_verify_regions_notrash_nocset, // no trash, no cset
Expand Down Expand Up @@ -1294,7 +1296,7 @@ void ShenandoahVerifier::help_verify_region_rem_set(Scanner* scanner, Shenandoah
ShenandoahOldGeneration* old_gen = _heap->old_generation();
assert(old_gen->is_mark_complete() || old_gen->is_parsable(), "Sanity");

ShenandoahMarkingContext* ctx = old_gen->is_mark_complete() ?old_gen->complete_marking_context() : nullptr;
ShenandoahMarkingContext* ctx = old_gen->is_mark_complete() ? old_gen->complete_marking_context() : nullptr;
ShenandoahVerifyRemSetClosure<Scanner> check_interesting_pointers(scanner, message);
HeapWord* from = r->bottom();
HeapWord* obj_addr = from;
Expand Down