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

[Bengt Rutisson]

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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141104/ede1343d/attachment.htm>