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

[Bengt Rutisson]

Hi Jungwoo,

Thanks for bringing this thread to life again.

On 2015-01-15 00:39, Jungwoo Ha wrote:
> Hi All,
>
> Let's revive the thread as it seems Bengt's change made in.
> So before I upload the webrev, is this what we've discussed last time?
>
> par_scan_state seems like a per-worker-thread data instead of 
> per-generation, but it doesn't seem to affect the performance in this 
> case.
>
> $ hg diff
> diff -r a184ee1d7172 
> src/share/vm/gc_implementation/parNew/parNewGeneration.cpp
> --- a/src/share/vm/gc_implementation/parNew/parNewGeneration.cppThu 
> Jan 08 12:08:22 2015 -0800
> +++ b/src/share/vm/gc_implementation/parNew/parNewGeneration.cppWed 
> Jan 14 15:34:57 2015 -0800
> @@ -1194,8 +1194,10 @@
>          return real_forwardee(old);
>      }
> -    new_obj = _next_gen->par_promote(par_scan_state->thread_num(),
> -                                       old, m, sz);
> +    if (!par_scan_state->promotion_failed()) {
> +      new_obj = _next_gen->par_promote(par_scan_state->thread_num(),
> +                                        old, m, sz);
> +    }
>      if (new_obj == NULL) {
>        // promotion failed, forward to self
>
>

Do you prefer the per thread check instead of the "global" state? Not 
sure which one is best. You say there is no performance difference 
between the two versions. Is that based on your test program that 
measures how long a GC that gets promotion failure takes? What about 
overall performance for normal GCs? The 
par_scan_state->promotion_failed() looks more expensive than just 
reading _promotion_failed.

Also, in your original patch you were guarding this check with a flag. 
Do you still want to do that?

Thanks,
Bengt


> Thanks,
> Jungwoo
>
> On Wed, Nov 26, 2014 at 11:38 PM, Bengt Rutisson 
> <bengt.rutisson at oracle.com <mailto:bengt.rutisson at oracle.com>> wrote:
>
>
>     Hi Kim and Jungwoo,
>
>     Sorry for not replying earlier.
>
>
>     On 2014-11-24 16:13, Kim Barrett wrote:
>
>         On Nov 23, 2014, at 10:08 PM, Kim Barrett
>         <kim.barrett at oracle.com <mailto:kim.barrett at oracle.com>> wrote:
>
>             On Nov 21, 2014, at 3:44 PM, Kim Barrett
>             <kim.barrett at oracle.com <mailto:kim.barrett at oracle.com>>
>             wrote:
>
>                 On Nov 20, 2014, at 9:27 PM, Jungwoo Ha
>                 <jwha at google.com <mailto:jwha at google.com>> wrote:
>
>                     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.
>
>                 There are multiple implementations of par_promote, for
>                 different
>                 old-space collectors.  (par_promote is a virtual
>                 function.)  At least
>                 some of the others may have similar issues (for
>                 example, at a quick
>                 look, ParallelOld appears to have a similar locking
>                 structure), and it
>                 seems like all could benefit from this short-circuiting.
>
>                 [One of the copy_to_survivor_space_XXX functions (the
>                 callers of
>                 par_promote) is used exclusively when CMS is the
>                 old-space collector,
>                 the other is used for other old-space collectors.]
>
>             Except I’ve now been reminded that ParNew doesn’t get used
>             with ParallelOld.
>             In fact, several combinations of old/new collectors were
>             deprecated in jdk8 and
>             are slated to be removed in jdk9 (some already have), and
>             it looks like ParNew
>             will only remain in conjunction with CMS, making the split
>             of ParNew’s
>             copy_to_survivor_space into two variants no longer needed,
>             and one of them
>             redundant, making this wrapper vs comment duplication vs
>             whatever issue moot.
>
>         To be clear, what I'm suggesting is that, in light of the
>         deprecation
>         and expected removal of some combinations with ParNew, only
>         copy_to_survivor_space_avoiding_promotion_undo() really needs the
>         proposed change, and copy_to_survivor_space_with_undo() could
>         be left
>         unmodified.
>
>
>     Yes, this is correct. I sent out the review to remove the support
>     for ParNew+SerialOld yesterday. That removes the
>     copy_to_survivor_space_avoiding_promotion_undo() method and folds
>     the copy_to_survivor_space_with_undo() method in to
>     copy_to_survivor_space() since that is now the only use case. See
>     the email thread "RFR (L): 8065972: Remove support for
>     ParNew+SerialOld and DefNew+CMS" for more details.
>
>     So, my suggestion would be to hold off with the patch for
>     JDK-8061259 a couple of days until my cleanup has been pushed.
>     That way there will only be one place to add the promotion failure
>     check.
>
>     I agree with Kim that a comment with a short explanation about the
>     intentional (safe) race would be good.
>
>     Thanks,
>     Bengt
>
>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150115/ce8d0e9c/attachment.htm>