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

[Jungwoo Ha]

http://cr.openjdk.java.net/~rasbold/8061259/webrev.04/
Thanks for the review.
On Tue Nov 04 2014 at 1:27:44 AM Bengt Rutisson <bengt.rutisson at oracle.com>
wrote:

>
> Hi Jungwoo,
>
> One final nit.
>
> The two setters don't need a return value. They could be void.
>
> 1125   bool set_promotion_failed()   { _has_promotion_failed = true; }
> 1126   bool reset_promotion_failed() { _has_promotion_failed = false; }
>
>
> Bengt
>
>
> On 2014-11-03 18:16, Jungwoo Ha wrote:
>
> Nice catch!
> PTAL
> http://cr.openjdk.java.net/~rasbold/8061259/webrev.03/
>
> On Thu Oct 30 2014 at 1:33:04 PM Bengt Rutisson <bengt.rutisson at oracle.com>
> wrote:
>
>>
>> Hi Jungwoo,
>>
>>
>> On 10/30/14 6:24 PM, Jungwoo Ha wrote:
>>
>> PTAL
>> http://cr.openjdk.java.net/~rasbold/8061259/webrev.02/
>>
>>
>> Thanks! Looks good except for one detail.
>>
>> 1125   bool set_promotion_failed()   { _has_promotion_failed = 1; }
>> 1126   bool reset_promotion_failed() { _has_promotion_failed = 0; }
>>
>> Since _has_promotion_failed is now a bool I don't think we should be
>> assigning 1 and 0 to it. We should be using true and false.
>>
>> Other than that it looks good to me.
>>
>> Thanks!
>>
>> Bengt
>>
>>
>>
>>
>> On Thu Oct 30 2014 at 12:28:19 AM Bengt Rutisson <
>> bengt.rutisson at oracle.com> wrote:
>>
>>>
>>> Hi again,
>>>
>>> One more minor thing.
>>>
>>> The methods has_promotion_failed(), set_promotion_failed() and
>>> reset_promotion_failed() are protected but they could be made private
>>> instead.
>>>
>>> Thanks,
>>> Bengt
>>>
>>>
>>> On 2014-10-30 08:09, Bengt Rutisson wrote:
>>>
>>>
>>>
>>> Hi Jungwoo,
>>>
>>> On 2014-10-29 23:51, Jungwoo Ha wrote:
>>>
>>>  PTAL
>>> http://cr.openjdk.java.net/~rasbold/8061259/webrev.01/
>>>
>>>  I've looked a bit at the webrev. A couple of comments:
>>>>
>>>> Why do you use OrderAccess methods for writing and reading the
>>>> _has_promo_failed flag in has_promo_failed() and set_promot_failed() ?
>>>>
>>>
>>>   I think that has no effect on x86, but I assumed that processors with
>>> weak memory model may want ordering of set/reset/has call.
>>>
>>>
>>> You don't need the OrderAccess methods for the weak memory models here
>>> either. You just race on reading the variable and if you see the "wrong"
>>> value you eventually take a lock (which will order all memory accesses) to
>>> read the variable properly.
>>>
>>> By removing the use of OrderAccess you can make
>>> ConcurrentMarkSweepGeneration::_has_promotion_failed a bool instead of a
>>> juint which simplifies the code a bit.
>>>
>>>
>>>  Can we write out the full word "promotion" instead of just "promo" in
>>>> the variables and methods?
>>>>
>>>
>>>   Done.
>>>
>>>   Can we change the name of the flag from UseCMSFastPromotionFailure to
>>>> CMSFastPromotionFailure? Most CMS flags start with CMS and I don't think we
>>>> need the "Use" prefix.
>>>>
>>>
>>>   Done.
>>>
>>>  What do you think about making the flag true by default? At least for
>>>> JDK 9. If we decide to backport to JDK 8 or 7 it might be a good idea to
>>>> keep the default value as false.
>>>>
>>>
>>>   Done.
>>> Let me know if there is anything for me to do to backport to JDK8 and 7.
>>>
>>>
>>> I think this fix would be worth backporting to JDK 8. I don't think
>>> there is much action required on your side. I created a backport bug for
>>> JDK 8 just to make sure that we don't forget it. It will be a little while
>>> before the 8 update repos are in a state to accept enhancements again. So,
>>> it is nice to have the backport bug to keep track of this.
>>>
>>> Backporting to JDK 7 requires some more work. Unless you have good
>>> arguments for why it is important to backport this to JDK 7 I don't think
>>> it is worth doing.
>>>
>>>
>>>  Did you find the information provided by _fast_promo_failure_hitcount
>>>> useful for debugging? If it not too useful I would consider removing it
>>>> since it is cluttering up the code a bit.
>>>>
>>>
>>>   I removed it.
>>> It was useful to for development, but I think it is no longer needed.
>>>
>>>
>>> Great. Thanks.
>>>
>>> One more comment. This code comment appears in two places just after we
>>> have taken the lock.
>>>
>>> 3365   if (CMSFastPromotionFailure && has_promotion_failed()) {
>>> 3366     // Caller must have checked already without synchronization.
>>> 3367     // Check again here while holding the lock.
>>> 3368     return NULL;
>>> 3369   }
>>>
>>> There is actually really no requirement that the caller must have
>>> checked has_promotion_failed() before calling the method. That's just an
>>> optimization. I think the first comment can be skipped and we just leave
>>> the second comment "// Check again here while holding the lock.". I would
>>> also suggest moving that comment up to the line just before the if
>>> statement.
>>>
>>> Thanks,
>>> Bengt
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141104/0f09f2bd/attachment.htm>