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

[John Coomes]

Stefan Karlsson (stefan.karlsson at oracle.com) wrote:
> 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?

Not at all, I'll do that.

>   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);
>    }
> }

I like that better too, so I'll do it.

> > 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?

Sure, easy enough.  Thanks for the review!

-John