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

Bengt Rutisson bengt.rutisson at oracle.com
Thu Oct 30 07:19:59 UTC 2014


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


More information about the hotspot-gc-dev mailing list