RFR: Shenandoah Partial GC with generational/LRU policies

[Roman Kennke]

Am 25.07.2017 um 20:51 schrieb Christine Flood:
>
>
> On Mon, Jul 24, 2017 at 4:07 PM, Roman Kennke <rkennke at redhat.com
> <mailto:rkennke at redhat.com>> wrote:
>
>     Am 24.07.2017 um 20:45 schrieb Christine Flood:
>>     http://cr.openjdk.java.net/~chf/GenerationalPartialGC/webrev.01/
>>     <http://cr.openjdk.java.net/%7Echf/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 else
>     1381 _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/
> <http://cr.openjdk.java.net/%7Echf/GenerationalPartialGC/webrev.02/>
>
> Good to go?

Ok