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

[Bengt Rutisson]

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/20141030/672f92cd/attachment.htm>