review request (m): 6330863 InfiniteList.java intermittent timeout

[Stefan Karlsson]

Hi John,

This looks good, but I have a couple of style change suggestions. See 
inlined:

On 02/14/2012 08:09 PM, John Coomes wrote:
> I'd appreciate reviews of these changes to fix a corner-case that
> can cause test timeouts with parallel old gc.
>
> The review is in two parts; the first part is a prereq that's easily
> separated so it's in a separate webrev to make reviews of the real fix
> a bit easier.
>
> 1) change the PS invoke methods to return an indicator of whether a gc
> was done:
>
> 	http://cr.openjdk.java.net/~jcoomes/6330863-invoke-return-val/

http://cr.openjdk.java.net/~jcoomes/6330863-invoke-return-val/src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp.frames.html

  229      -  bool scavenge_was_done = PSScavenge::invoke_no_policy();
       229 +  const bool need_full_gc = !PSScavenge::invoke_no_policy() ||
       230 +    policy->should_full_GC(heap->old_gen()->free_in_bytes());
       231 +  bool did_full_gc = false;

IMHO, inlining the invoke_no_policy call here makes it harder to read. 
Would you mind keeping scavenge_was_done and use it on line 229?


  223-250 ...

I think it would be good for readability to extract counter update code 
out of the full GC calling code. For example:
if (UsePerfData) {
  PSGCAdaptivePolicyCounters* const counters = heap->gc_policy_counters();
  if (need_full_gc) {
    counters->update_full_follows_scavenge(full_follows_scavenge);
  } else {
    counters->update_full_follows_scavenge(0);
  }
}

if (need_full_gc) {
   GCCauseSetter gccs(heap, GCCause::_adaptive_size_policy);
   CollectorPolicy* cp = heap->collector_policy();
   const bool clear_all_softrefs = cp->should_clear_all_soft_refs();

   if (UseParallelOldGC) {
     did_full_gc = PSParallelCompact::invoke_no_policy(clear_all_softrefs);
   } else {
     did_full_gc = PSMarkSweep::invoke_no_policy(clear_all_softrefs);
   }
}

>
> 2) adjust the allocation policy:
>
> 	http://cr.openjdk.java.net/~jcoomes/6330863-death-march/

http://cr.openjdk.java.net/~jcoomes/6330863-death-march/src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.cpp.frames.html


  421       if (result != NULL) return result;

and

  425       if (result != NULL) return result;

Could you keep the return statements on a separate line?


StefanK

>
> -John