RFR: JDK-8061259: ParNew promotion failed is serialized on a lock

[Kim Barrett]

On Nov 19, 2014, at 12:13 PM, Jungwoo Ha <jwha at google.com> wrote:
> 
> http://cr.openjdk.java.net/~rasbold/8061259/webrev.06/
> 
> PTAL. 
> 
> On Mon, Nov 17, 2014 at 7:57 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
> On Nov 17, 2014, at 5:11 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> >
> >> I think this does leave open the possibility of moving the
> >> _promotion_failed flag check earlier in copy_to_survivor_space_XXX(),
> >> […]
> >>
> >> I'm not sure it would be overall beneficial though, […]
> >
> > Yes, there is probably room for more improvement here. I think adding the check in the copy_to_survivor_space_XXX_undo() methods is a clear improvement in many cases. So, doing that as a first step is a good start. We can consider moving it earlier as a future enhancement.
> 
> Agreed.

Mostly looks good.  A couple of nits:

------------------------------------------------------------------------------ 
src/share/vm/gc_implementation/parNew/parNewGeneration.cpp  
1199     // We can skip par_promote if promotion has been failed already.

"... promotion has been failed already."
=>
"... promotion has already failed."

There is similar awkward grammar in the comment on the other call, but
see below.

------------------------------------------------------------------------------
src/share/vm/gc_implementation/parNew/parNewGeneration.cpp 
1199     // We can skip par_promote if promotion has been failed already.
1200     // Checking _promotion_failed can be racy, but the worse case is going
1201     // through the slow path of calling par_promote, which will still correctly
1202     // handle the promotion failure.

1323     // Skip this step if promotion has already been failed.

I dislike these two very different comments for the same thing.  On
the other hand, I dislike duplication of information.  And a reference
to the earlier comment from the later one can also become stale.

Maybe add a (private) helper function that is a wrapper around the
call to par_promote, with the flag test and associated comment in the
wrapper?  [Though this would become obsolete if we later decide we can
move the flag test earlier.] Or just duplicate the more informative
comment (ick!).

Bengt, what do you think?