Skip to content

8369048: GenShen: Defer ShenFreeSet::available() during rebuild #27612

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+37 −17
Open

8369048: GenShen: Defer ShenFreeSet::available() during rebuild #27612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
56291b4
Add support for freeset rebuild lock
kdnilsen Oct 2, 2025
0944fa8
fix whitespace
kdnilsen Oct 2, 2025
623b0f0
Treat traditional Shenandoah same as Generational Shenandoah
kdnilsen Oct 5, 2025
99d0175
reduce gratuitous diffs from tip
kdnilsen Oct 5, 2025
bec73da
revert introduction of RebuildLock
kdnilsen Oct 6, 2025
1a5e483
available() returns previous value if called during freeset rebuild
kdnilsen Oct 7, 2025
717e7da
remove obsolete assertion
kdnilsen Oct 7, 2025
78739b5
Revert "remove obsolete assertion"
kdnilsen Oct 7, 2025
d9a8a51
Revert "available() returns previous value if called during freeset r…
kdnilsen Oct 7, 2025
a689839
Revert "revert introduction of RebuildLock"
kdnilsen Oct 7, 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
19 changes: 17 additions & 2 deletions src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,12 @@

#include "gc/shenandoah/shenandoahHeap.hpp"
#include "gc/shenandoah/shenandoahHeapRegionSet.hpp"
#include "gc/shenandoah/shenandoahLock.hpp"
#include "gc/shenandoah/shenandoahSimpleBitMap.hpp"

typedef ShenandoahLock ShenandoahRebuildLock;
typedef ShenandoahLocker ShenandoahRebuildLocker;

// Each ShenandoahHeapRegion is associated with a ShenandoahFreeSetPartitionId.
enum class ShenandoahFreeSetPartitionId : uint8_t {
Mutator, // Region is in the Mutator free set: available memory is available to mutators.
Expand Down Expand Up @@ -234,7 +238,7 @@ class ShenandoahRegionPartitions {
// Return available_in assuming caller does not hold the heap lock. In production builds, available is
// returned without acquiring the lock. In debug builds, the global heap lock is acquired in order to
// enforce a consistency assert.
Comment on lines 238 to 240
Copy link
Member

Choose a reason for hiding this comment

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

Can the comment be simplified to:

// Return bytes `available` in the given `partition`
// while holding the `rebuild_lock`.

Don't say anything about the heap lock in the API comment. Rather, in the part that is ifdef ASSERT where you take the heap lock (line ~244), say:

//  Acquire the heap lock to get a consistent
// snapshot to check assert.

Copy link
Member

Choose a reason for hiding this comment

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

As I write this, I realize that in the most general case where two threads may call these API's independently in a fastdebug build, you could theoretically get into a deadlock because they attempted to acquire locks in different orders (this possibility exists -- statically -- only in the fastdebug builds).

The general MuteLocker machinery has ranked mutexes to avoid such situations through static ranking and checks while acquiring locks (in debug builds as a way of potentially catching such situations and flagging them).

With such ranking though this code would assert because the locks are acquired in different order between here and elsewhere.

In product builds you are fine because the rebuild lock acts as a "leaf lock" (in hotspot parlance). But there seems to be a definite possibility of deadlock in debug builds if/when the rebuild is attempted by one thread while another checks available and attempts to acquire the heap lock to check the assertion. You could solve it by acquiring the heap lock before calling the work method where the assertion check is done.

However, I'd be much more comfortable if we used some form of lock rank framework, unless it was utterly impossible to do so for some reason. (Here it was easy to spot the lock order inversion because it was in the code. Of course, if a debug build deadlocked you would also figure out the same, but having lock ordering gives you a quick and easy way to verify if there's potential for trouble.)

Not sure of the history of ShenandoahLock or why the parallel infra to MutexLocker was introduced (perhaps for allowing some performance/tunability), but might be worthwhile to see if we want to build lock rank checks in for robustness/maintainability.

inline size_t available_in_not_locked(ShenandoahFreeSetPartitionId which_partition) const {
inline size_t available_in_locked_for_rebuild(ShenandoahFreeSetPartitionId which_partition) const {
assert (which_partition < NumPartitions, "selected free set must be valid");
shenandoah_assert_not_heaplocked();
#ifdef ASSERT
Expand Down Expand Up @@ -316,6 +320,9 @@ class ShenandoahFreeSet : public CHeapObj<mtGC> {
ShenandoahHeap* const _heap;
ShenandoahRegionPartitions _partitions;

// This locks the rebuild process (in combination with the global heap lock)
Copy link
Member

Choose a reason for hiding this comment

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

Explain the role of this & the global heap lock vis-a-vis the rebuild process.

Also may be call it _rebuild_lock, rather than just _lock.

ShenandoahRebuildLock _lock;

HeapWord* allocate_aligned_plab(size_t size, ShenandoahAllocRequest& req, ShenandoahHeapRegion* r);

// Return the address of memory allocated, setting in_new_region to true iff the allocation is taken
Expand Down Expand Up @@ -415,6 +422,11 @@ class ShenandoahFreeSet : public CHeapObj<mtGC> {

ShenandoahFreeSet(ShenandoahHeap* heap, size_t max_regions);


ShenandoahRebuildLock* lock() {
Copy link
Member

Choose a reason for hiding this comment

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

rebuild_lock() instead?

return &_lock;
}

// Public because ShenandoahRegionPartitions assertions require access.
inline size_t alloc_capacity(ShenandoahHeapRegion *r) const;
inline size_t alloc_capacity(size_t idx) const;
Expand Down Expand Up @@ -480,7 +492,10 @@ class ShenandoahFreeSet : public CHeapObj<mtGC> {
// locked action can be seen by these unlocked functions.
inline size_t capacity() const { return _partitions.capacity_of(ShenandoahFreeSetPartitionId::Mutator); }
inline size_t used() const { return _partitions.used_by(ShenandoahFreeSetPartitionId::Mutator); }
inline size_t available() const { return _partitions.available_in_not_locked(ShenandoahFreeSetPartitionId::Mutator); }
inline size_t available() {
ShenandoahRebuildLocker locker(lock());
return _partitions.available_in_locked_for_rebuild(ShenandoahFreeSetPartitionId::Mutator);
}

HeapWord* allocate(ShenandoahAllocRequest& req, bool& in_new_region);

Expand Down
10 changes: 5 additions & 5 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 @@ -1153,17 +1153,17 @@ ShenandoahGenerationalHeap::TransferResult ShenandoahFullGC::phase5_epilog() {
}

heap->collection_set()->clear();
size_t young_cset_regions, old_cset_regions;
size_t first_old, last_old, num_old;
heap->free_set()->prepare_to_rebuild(young_cset_regions, old_cset_regions, first_old, last_old, num_old);

size_t young_cset_regions, old_cset_regions, first_old, last_old, num_old;
ShenandoahFreeSet* free_set = heap->free_set();
ShenandoahRebuildLocker rebuild_locker(free_set->lock());
Copy link
Member

Choose a reason for hiding this comment

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

Should you not create a scope around lines 1158 to line 1167, since you don't want to hold the rebuild lock as soon as the rebuild is done (i.e. immediately following finish_rebuild())?

Copy link
Member

Choose a reason for hiding this comment

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

May be it doesn't matter, since no one else is running during a full gc who needs to query available()?

free_set->prepare_to_rebuild(young_cset_regions, old_cset_regions, first_old, last_old, num_old);
// We also do not expand old generation size following Full GC because we have scrambled age populations and
// no longer have objects separated by age into distinct regions.
if (heap->mode()->is_generational()) {
ShenandoahGenerationalFullGC::compute_balances();
}

heap->free_set()->finish_rebuild(young_cset_regions, old_cset_regions, num_old);
free_set->finish_rebuild(young_cset_regions, old_cset_regions, num_old);

// Set mark incomplete because the marking bitmaps have been reset except pinned regions.
heap->global_generation()->set_mark_incomplete();
Expand Down
6 changes: 4 additions & 2 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 @@ -745,14 +745,16 @@ void ShenandoahGeneration::prepare_regions_and_collection_set(bool concurrent) {
{
ShenandoahGCPhase phase(concurrent ? ShenandoahPhaseTimings::final_rebuild_freeset :
ShenandoahPhaseTimings::degen_gc_final_rebuild_freeset);
ShenandoahFreeSet* free_set = heap->free_set();
ShenandoahHeapLocker locker(heap->lock());
ShenandoahRebuildLocker rebuild_locker(free_set->lock());
size_t young_cset_regions, old_cset_regions;

// We are preparing for evacuation. At this time, we ignore cset region tallies.
size_t first_old, last_old, num_old;
heap->free_set()->prepare_to_rebuild(young_cset_regions, old_cset_regions, first_old, last_old, num_old);
free_set->prepare_to_rebuild(young_cset_regions, old_cset_regions, first_old, last_old, num_old);
// Free set construction uses reserve quantities, because they are known to be valid here
heap->free_set()->finish_rebuild(young_cset_regions, old_cset_regions, num_old, true);
free_set->finish_rebuild(young_cset_regions, old_cset_regions, num_old, true);
}
}

Expand Down
6 changes: 4 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,6 +424,7 @@ jint ShenandoahHeap::initialize() {

// We are initializing free set. We ignore cset region tallies.
size_t first_old, last_old, num_old;
ShenandoahRebuildLocker rebuild_locker(_free_set->lock());
_free_set->prepare_to_rebuild(young_cset_regions, old_cset_regions, first_old, last_old, num_old);
_free_set->finish_rebuild(young_cset_regions, old_cset_regions, num_old);
}
Expand Down Expand Up @@ -2575,6 +2576,7 @@ void ShenandoahHeap::rebuild_free_set(bool concurrent) {
ShenandoahPhaseTimings::final_update_refs_rebuild_freeset :
ShenandoahPhaseTimings::degen_gc_final_update_refs_rebuild_freeset);
ShenandoahHeapLocker locker(lock());
ShenandoahRebuildLocker rebuild_locker(_free_set->lock());
size_t young_cset_regions, old_cset_regions;
size_t first_old_region, last_old_region, old_region_count;
_free_set->prepare_to_rebuild(young_cset_regions, old_cset_regions, first_old_region, last_old_region, old_region_count);
Expand All @@ -2600,8 +2602,8 @@ void ShenandoahHeap::rebuild_free_set(bool concurrent) {

// Total old_available may have been expanded to hold anticipated promotions. We trigger if the fragmented available
// memory represents more than 16 regions worth of data. Note that fragmentation may increase when we promote regular
// regions in place when many of these regular regions have an abundant amount of available memory within them. Fragmentation
// will decrease as promote-by-copy consumes the available memory within these partially consumed regions.
// regions in place when many of these regular regions have an abundant amount of available memory within them.
// Fragmentation will decrease as promote-by-copy consumes the available memory within these partially consumed regions.
//
// We consider old-gen to have excessive fragmentation if more than 12.5% of old-gen is free memory that resides
// within partially consumed regions of memory.
Expand Down
13 changes: 7 additions & 6 deletions src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,13 +509,14 @@ void ShenandoahOldGeneration::prepare_regions_and_collection_set(bool concurrent
ShenandoahGCPhase phase(concurrent ?
ShenandoahPhaseTimings::final_rebuild_freeset :
ShenandoahPhaseTimings::degen_gc_final_rebuild_freeset);
ShenandoahFreeSet* free_set = heap->free_set();
ShenandoahHeapLocker locker(heap->lock());
size_t cset_young_regions, cset_old_regions;
size_t first_old, last_old, num_old;
heap->free_set()->prepare_to_rebuild(cset_young_regions, cset_old_regions, first_old, last_old, num_old);
// This is just old-gen completion. No future budgeting required here. The only reason to rebuild the freeset here
// is in case there was any immediate old garbage identified.
heap->free_set()->finish_rebuild(cset_young_regions, cset_old_regions, num_old);
ShenandoahRebuildLocker rebuild_locker(free_set->lock());
size_t cset_young_regions, cset_old_regions, first_old, last_old, num_old;
free_set->prepare_to_rebuild(cset_young_regions, cset_old_regions, first_old, last_old, num_old);
// This is completion of old-gen marking. We rebuild in order to reclaim immediate garbage and to
// prepare for subsequent mixed evacuations.
free_set->finish_rebuild(cset_young_regions, cset_old_regions, num_old);
}
}

Expand Down