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

[Jungwoo Ha]

If we are to put a wrapper around, why not just go with the original change?
I don't see adding a single field on a nearly singleton class is a bad
thing.
The changes are well wrapped inside par_promote.
It is the fast path of the promotion, so I think it is more readable.
We don't need a long explanation to the code reader why we are allowing the
data-race.


On Thu, Nov 20, 2014 at 2:12 PM, Kim Barrett <kim.barrett at oracle.com> wrote:

> 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?
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141120/04a2402e/attachment.htm>