RFR: Refactor to group marking bitmap and TAMS structure in one class ShenandoahMarkingContext

[Aleksey Shipilev]

On 07/17/2018 10:20 PM, Roman Kennke wrote:
> This is a long overdue refactoring. It groups the two structures
> involved in marking (bitmap + TAMS) into one class
> ShenandoahMarkingContext. This allows for some interesting improvements:
> - Easy to keep in sync (e.g. simpler swapping and reasoning-about)
> - Can keep actual arrays const
> - Can pass around the context (see upcoming patch)
> - Code related to marking and other bookkeeping is now grouped together
> in one class
> - Reduce duplications next vs. complete versions of methods
> 
> The code now makes obvious where we have extra dereferences. I felt the
> itch to reduce XX_marking_context() calls in a bunch of places, but
> wanted to keep this a mechanical refactoring as much as possible.
> Upcoming patch resolves some of the hottest indirections. In any case,
> it's not worse than it was before (it only looks worse).
> 
> Testing: tier3_gc_shenandoah ok
> 
> http://cr.openjdk.java.net/~rkennke/refactor-marking-context/webrev.00/

Looks cute!

 *) This method is probably "swap_mark_contexts" now:
  1994 void ShenandoahHeap::swap_mark_bitmaps() {

 *) ShIsAlive, ShForwardedIsAlive, ShIsMarkedNextClosure closures are also performance-sensitive...

 *) context()->mark_bit_map()->clear_range_large(...) can probably be put in utility method, e.g.
context()->clear_bitmap(...) or something. In fact, mark_bit_map() should only be used when
something wants the naked bitmap, e.g. Verifier.

 *) Seems to me, you can use local variables a lot to both simplify the code, and micro-optimize. E.g.:

  if (heap->is_bitmap_slice_committed(region)) {
    if (_traversal_set.is_in(i)) {
      heap->next_marking_context()->set_top_at_mark_start(region->region_number(), region->top());
      region->clear_live_data();
      assert(heap->next_marking_context()->is_bitmap_clear_range(region->bottom(), region->end()),
"bitmap for traversal regions must be cleared");
    } else {
      // Everything outside the traversal set is always considered live.
      heap->next_marking_context()->set_top_at_mark_start(region->region_number(), region->bottom());
    }
  }

...is:

  ShenandoahMarkingContext* ctx = heap->next_marking_context();
  if (heap->is_bitmap_slice_committed(region)) {
    if (_traversal_set.is_in(i)) {
      ctx->set_top_at_mark_start(region->region_number(), region->top());
      region->clear_live_data();
      assert(ctx->is_bitmap_clear_range(region->bottom(), region->end()), "bitmap for traversal
regions must be cleared");
    } else {
      // Everything outside the traversal set is always considered live.
      ctx->set_top_at_mark_start(region->region_number(), region->bottom());
    }
  }

Thanks,
-Aleksey