RFR: Shenandoah Partial GC with generational/LRU policies

[Christine Flood]

On Mon, Jul 24, 2017 at 4:07 PM, Roman Kennke <rkennke at redhat.com> wrote:

> Am 24.07.2017 um 20:45 schrieb Christine Flood:
>
> http://cr.openjdk.java.net/~chf/GenerationalPartialGC/webrev.01/
>
> I've made the changes Shade requested and passed all of the tests,
> including the suggested additions, with the exception
> of gc/shenandoah/ShenandoahJNICritical.java which is also broken in a clean
> build.
>
> Christine
>
> - ShenandoahHeuristics::_bytes_allocated_after_last_gc is unused now.
> Remove?
>
OK

> - I would find it nicer to make the compare_by_* private static members of
> ShenandoahHeuristics.
>
They can't be private because they are used by subclasses.  I want them to
be available for future policies.

> - I wonder if the is_in() check should be in the normal add_region() path.
> After all it's called *Set, I don't think we ever want duplicates, and
> is_in() is cheap now. ?
>
I don't think so.  I hate adding the extra checks where they aren't needed.

> - I don't really like all the timestamp stuff in ShenandoahHeap. Sounds
> like it should be in ShenandoahHeuristics or appropriate subclass?
>
Except it really is a part of all gcs, including full gcs, and therefore to
me it belongs in shenandoahheap.

Also, is_conc_gc_in_progress() should be easier to do using the existing
> *_in_progress() accessors somehow? E.g.:
>
> bool is_conc_gc_in_progress() {
>
>   return concurrent_mark_in_progress() || is_evacuation_in_progress() ||
> is_update_refs_in_progress();
>
> }
>
 OK

> - ShenandoahHeuristics::maybe_add_heap_region() ... should probably be a
> member of ShenandoahCollectionSet? And dunno if we can come up with a
> slightly more descriptive name? Can't quickly think of one though.
>
It requires information about the regions so I think it belongs in
shenandoah heuristics.

> - PartialHeuristics::from_idxs should be PartialHeuristics::_from_idxs
>
> OK

> 1376 void ShenandoahCollectorPolicy::choose_collection_set(ShenandoahCollectionSet* collection_set,1377                                                       bool minor) {1378   if (minor)1379     _minor_heuristics->choose_collection_set(collection_set, minor);1380   else1381     _heuristics->choose_collection_set(collection_set, minor);
> 1382 }
>
> There's no need to pass down 'minor', is there? It's not used downstream anyway.
>
> OK

> 59     } else if (heap->shenandoahPolicy()->should_start_partial_gc()) {60       if (! heap->need_update_refs())
> 61         service_partial_cycle();
>
> You added a check for need_update_refs() here. My idea is that heuristics that do partial cycles should never need update refs in between. Is this possible now?
>
> We are mixing partial (generational) collections with full (concurrent
marking and everything) cycles.  I moved the check into
should_start_partial_gc.

> There's a whitespace changes that's not for the better IMO: 49   void do_oop_work(T* p) {_partial_gc->process_oop<T, false>(p, _thread, _queue);}
>
> Fair enough

>
>
> That's all for now :-)
>
> http://cr.openjdk.java.net/~chf/GenerationalPartialGC/webrev.02/

Good to go?


Christine