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

Bengt Rutisson bengt.rutisson at oracle.com
Wed Nov 5 09:45:07 UTC 2014


On 2014-11-04 17:16, Jungwoo Ha wrote:
> http://cr.openjdk.java.net/~rasbold/8061259/webrev.04/ 
> <http://cr.openjdk.java.net/%7Erasbold/8061259/webrev.04/>

Looks good.

I can sponsor this change. We need one more reviewer to look at it 
before I can push.

Bengt


> Thanks for the review.
> On Tue Nov 04 2014 at 1:27:44 AM Bengt Rutisson 
> <bengt.rutisson at oracle.com <mailto: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/
>>     <http://cr.openjdk.java.net/%7Erasbold/8061259/webrev.03/>
>>
>>     On Thu Oct 30 2014 at 1:33:04 PM Bengt Rutisson
>>     <bengt.rutisson at oracle.com <mailto: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/
>>>         <http://cr.openjdk.java.net/%7Erasbold/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
>>>         <mailto: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/
>>>>>             <http://cr.openjdk.java.net/%7Erasbold/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: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20141105/5274e552/attachment-0001.html>


More information about the hotspot-gc-dev mailing list