RFR: 8353115: GenShen: mixed evacuation candidate regions need accurate live_data [v13]

[Kelvin Nilsen]

On Mon, 10 Nov 2025 21:54:14 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Kelvin Nilsen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 58 commits:
>> 
>>  - Fix mistaken merge resolution
>>  - Merge remote-tracking branch 'jdk/master' into fix-live-data-for-mixed-evac-candidates
>>    
>>    The resulting fastdebug build has 64 failures.  I need to debug these.
>>    Probably introduced by improper resolution of merge conflicts
>>  - fix error in merge conflict resolution
>>  - Merge remote-tracking branch 'jdk/master' into fix-live-data-for-mixed-evac-candidates
>>  - rework CompressedClassSpaceSizeinJmapHeap.java
>>  - fix errors in CompressedClassSpaceSizeInJmapHeap.java
>>  - Add debug instrumentation to CompressedClassSpaceSizeInJmapHeap.java
>>  - fix two indexing bugs
>>  - add an assert to detect suspected bug
>>  - Remove debug scaffolding
>>  - ... and 48 more: https://git.openjdk.org/jdk/compare/c272aca8...16cd6f8a
>
> src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp line 165:
> 
>> 163: }
>> 164: 
>> 165: inline size_t ShenandoahHeapRegion::get_live_data_words(ShenandoahMarkingContext* ctx, size_t index) const {
> 
> Why do we want to change this signature? If `index` is always `this->_index` why go through the trouble to pass a member field to a member function on the same instance? I have a similar sentiment about passing `ShenandoahMarkingContext` through the function. Should we have a member `ShenandoahMarkingContext* _marking_context`? Changing this signature creates a lot of noise on the PR and it's not clear to me why we would do this.

Good call out.  Am willing to back this change out. Motivation for this change is that we were seeing some performance regression in this PR.  At first, I thought this was due to miscomputation of get_live_data_words(), but I confirmed through further testing that the results from get_live_data_words() were the same before and after this PR.

So I concluded that the "explanation" for performance regression is that it now takes longer for us to compute get_live_data_words().  The original implementation was:

  return AtomicAccess::load(&_live_data)

The new implementation added:

  Find the marking context by fetching this from ShenandoahHeap::heap()
  Find tams by consulting the marking context with region, which has to indirection through region to find index

I found that passing this information into the function rather than having the function recompute it brought us back to par with performance of master.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24319#discussion_r2515338461