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

[Kelvin Nilsen]

On Wed, 9 Apr 2025 17:51:06 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> My experiment with an initial attempt at this failed with over 60 failures.  The "problem" is that we often consult get_live_data() in contexts from which it is "not appropriate" to add (top- TAMS) to the atomic volatile ShenandoahHeapRegion::_live_data() .  I think most of these are asserts.  I have so far confirmed that there are at least two different places that need to be fixed.  Not sure how many total scenarios.
>> 
>> I'm willing to move forward with changes to the failing asserts to make this change work.  I think the code would be cleaner with your suggested refactor.  It just makes this PR a little more far-reaching than the original.
>> 
>> See the most recent commit on this PR to see the direction this would move us.  Let me know if you think I should move forward with more refactoring, or revert this most recent change.
>> 
>> Thanks.
>
> It does look simpler. Do you have an example of one of the failing asserts? 
> 
> One thing I hadn't considered is how "hot" `ShenandoahHeapRegion::get_live_data_words` is. Is there going to be a significant performance hit if we make this method do more work? It does look like this method is called frequently.

Examples:
  FullGC worker:
   void ShenandoahMCResestCompleteBitmapTask::work(uint worker_id) {
    ShenandoahParallelWorkerSession worker_session(worker_id);
    ShenandoahHeapRegion* region = _regions.next();
    ShenandoahHeap* heap = ShenandoahHeap::heap();
    ShenandoahMarkingContext* const ctx = heap->complete_marking_context();
    while (region != nullptr) {
      if (heap->is_bitmap_slice_committed(region) && !region->is_pinned() && region->has_marked()) {
        //  kelvin replacing has_live() with new method has_marked() because has_live() calls get_live_data_words()
        //  and pointer_delta() asserts out because TAMS is not less than top().  has_marked() does what has_live()
        //  used to do...
        ctx->clear_bitmap(region);
      }
      region = _regions.next();
    }
  }

ShenandoahInitMarkUpdateRegionStateClosure::heap_region_do() {
-    assert(!r->has_live(), "Region %zu should have no live data", r->index());
+    assert(!r->has_marked(), "Region %zu should have no marked data", r->index());

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

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