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

[Bengt Rutisson]

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/7cdeccd1/attachment.htm>