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

[Jungwoo Ha]

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.cpp Thu Jan 08
12:08:22 2015 -0800
+++ b/src/share/vm/gc_implementation/parNew/parNewGeneration.cpp Wed 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


Thanks,
Jungwoo

On Wed, Nov 26, 2014 at 11:38 PM, Bengt Rutisson <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> wrote:
>>
>>> On Nov 21, 2014, at 3:44 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>
>>>> On Nov 20, 2014, at 9:27 PM, Jungwoo Ha <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/20150114/806e2371/attachment.htm>