RFR: Generational/LRU partial heuristics take two...

[Aleksey Shipilev]

On 07/20/2017 10:38 PM, Christine Flood wrote:
> http://cr.openjdk.java.net/~chf/GenerationalPartialGC/webrev.00/

=== General comments:

 *) Add new heuristics to the tests. Grep hotspot/test/gc for
"ShenandoahGCHeuristics=partial", and add lines with lru/generational there, and
then try to pass hotspot_gc_shenandoah with it

===  Major issues:

*) Catastrophic underflow opportunity here (size_t is unsigned) in
PartialHeuristics:

  815   bool result = (used - previous_used) * 100 / capacity ...

*) PartialHeuristics destructor leaks memory, should pass from_idxs!

  794     FREE_C_HEAP_ARRAY(size_t, ShenandoahPartialInboundThreshold);

*) LRUPartialHeuristics, missing { on this line, making the whole block
nonsensical! See how indenting does not match, pointing at trouble.

  1006                                  ShenandoahPartialInboundThreshold))

*) Why timestamps are set in  ShenandoahHeapRegionSet::next()? The comments for
timestamps say they are set "as regions are discarded". But SHRS::next is just
the iteration that can be used anywhere. This is calling for trouble.


=== Stylistic:

*) ShenandoahHeuristics, stray whitespace at the end:

 205   virtual void choose_collection_set(ShenandoahCollectionSet*
collection_set, bool minor = false );

*) ShenandoahHeuristics, don't need this:

 272     ShenandoahHeap* heap = ShenandoahHeap::heap();

*) ShenandoahHeuristics, predicate order in the same method:

   if (!hr->is_humongous() && !hr->is_pinned() && !hr->is_empty() &&
        hr->has_live() && !collection_set->is_in(hr)) {

PartialHeuristics:

*) Can be private:

 781   size_t* from_idxs;

*) Too long log_info lines, plus missing punctuation in messaging. Also, braces.
Do e.g.:

  log_info(gc,ergo)("Skip partial GC. Capacity = " SIZE_FORMAT
         ", used = " SIZE_FORMAT ", previous GC used = " SIZE_FORMAT
         ", threshold = " SIZE_FORMAT,
          capacity, used, previous_used, ShenandoahAllocationThreshold);

GenerationalPartialHeuristics:

*) Braces:

  886     for (size_t i = 0; i < active; i++)
  887       sorted_regions.add_region(regions->get(i));

*) Stray newlines:

  893
  894

*) "\n" is not needed. Should have (gc,ergo) tag. log_info(gc,ergo) format
should match the one from parent PartialHeuristics.

  917     log_info(gc)("arbitrary = "SIZE_FORMAT" active = "SIZE_FORMAT" ...

*) Heap is available in local variable at this point:

  933     size_t threshold = ShenandoahHeap::heap()->regions()->active_regions()

*) It is saner to multiply everything first, and then divide, to improve accuracy?

  933   size_t threshold = ShenandoahHeap::heap()->regions()->active_regions() *
  934       percentage_young / 100 *
  935       ShenandoahHeapRegion::region_size_bytes();

*) Whitespace:

  949   bool can_do_partial_gc() { return true;}


LRUPartialHeuristics:

*) This is unused:

  971     outputStream *out = Log(gc)::info_stream();

*) Stray newlines:

  980
  981

*) log_info should have (gc,ergo) tag. Also, what is "arbitrary"? Needs short
descriptive name. "\n" is not needed. log_info(gc,ergo) format should match the
one from parent PartialHeuristics.

 1016       log_info(gc)("arbitrary = "SIZE_FORMAT" ...)

*) Heap is available in local variable at this point:

 1031   size_t threshold = ShenandoahHeap::heap()->regions()->active_regions() *
 1035     size_t minimum = ShenandoahHeap::heap()->regions()->active_regions() *


ShenandoahCollectorPolicy:

*) I guess we can say "false" on behalf of major heuristics here?

   bool ShenandoahCollectorPolicy::can_do_partial_gc() {
     if (_minor_heuristics != NULL)
       return _minor_heuristics->can_do_partial_gc();
     }
     return false; // no minor, no partial gc
   }

*) Whitespace:

  264   ShenandoahHeuristics* heuristics() { return _heuristics;}


ShenandoahConcurrentThread:

*) Why this check here?

   60       if (! heap->need_update_refs())
   61         service_partial_cycle();


ShenandoahConnectionMatrix:

*) Should mention the length for format here, "%8s"?:

   65   st->print_cr("%8s, %10s, %10s, %10s, %8s, %8s, %s, %s",
   66                "Region", "Live", "Used", "Garbage",
   67                "TS_Start", "TS_End", "Refcnt", "Referenced by");
   68

*) Should match the format length for the header, "%8.4f", not "%4.4f"?

   82         st->print("%8u, "SIZE_FORMAT_W(10)", "SIZE_FORMAT_W(10)",
"SIZE_FORMAT_W(10)", %4.4F, %4.4F, %8u, {",

ShenandoahHeap:

*) Excess log_info(gc):

 1976   log_info(gc)("Doing Partial Collection");

*) Whitespace, and also please indent them:

  double timestamp_at_last_gc_end()   const { return _timestamp_...; }
  double timestamp_at_last_gc_start() const { return _timestamp_...; }
  size_t used_at_last_gc()            const { return _used_at_last_gc; }

ShenandoahHeapRegion:

*) Need to set format length:

  148   st->print("|FTS %lf",  first_timestamp());
  149   st->print("|LTS %lf",  last_timestamp());

*) Stray newline:

  284

*) Whitespace and indents:

  185   double first_timestamp() const { return _first_timestamp;}
  186   double last_timestamp() const { return _last_timestamp;}
  187   void   set_first_timestamp() {_first_timestamp = os::elapsedTime();}
  188   void   set_last_timestamp() { _last_timestamp = os::elapsedTime();}


Partial GC:

*) Whitespace difference marks these lines as changed:

   49   void do_oop_work(T* p) {_partial_gc->process_oop<T, false> ...

  244   log_info(gc,ergo)("Got "SIZE_FORMAT" collection set regions, ...

*) Stray whitespace differences:

   93
   95

*) Unused:

  215   outputStream *out = Log(gc)::info_stream();

*) Use _heap->shenandoahPolicy() instead:

  212   ShenandoahCollectorPolicy* policy = (ShenandoahCollectorPolicy*)
_heap->collector_policy();

*) Not sure why DPT::update_pointers and task_queues->clear() are now outside
the block, around L300-305? At least indenting is not right.

Thanks,
-Aleksey