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

[Bengt Rutisson]

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


>