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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141105/5274e552/attachment.htm>
More information about the hotspot-gc-dev
mailing list