[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8181859: Monitor deflation is not checked in cleanup path
From:       coleen.phillimore () oracle ! com
Date:       2017-06-16 14:06:09
Message-ID: b87dde92-c3b0-bb65-d2e5-1fff28be8d3b () oracle ! com
[Download RAW message or body]

This looks good to me but also one more vote for parentheses in:

http://cr.openjdk.java.net/~rehn/8181859/2/webrev/src/share/vm/runtime/synchronizer.cpp.udiff.html

Nice improvement!
Coleen

On 6/15/17 3:59 AM, Robbin Ehn wrote:
> Hi all,
>
> Thanks for the reviews!
>
> On 06/14/2017 10:19 PM, serguei.spitsyn@oracle.com wrote:
>>>
>>> src/share/vm/runtime/globals.hpp
>>>     L1187   experimental(intx, MonitorUsedDeflationThreshold, 
>>> 90,                     \
>>>     L1188           "Percentage of used monitors before trigger 
>>> cleanup "             \
>>>     L1189           "safepoint which deflates monitors (0 is 
>>> off)"                    \
>>>     L1190           "The check is performed on 
>>> GuaranteedSafepointInterval")          \
>>>     L1191           range(0, 
>>> 100)                                                     \
>>>
>>>         nit - indent for L1188-91 should be after the '(' on L1187.
>>>
>>>         typo - 'trigger' -> 'triggering'
>>>         typo - 'off)' -> 'off).' (add period)
>>>         typo - 'Interval")' -> 'Interval.")' (add period)
>
> Fixed
>
>>>
>>> src/share/vm/runtime/synchronizer.hpp
>>>     nit - please update copyright year to 2017.
>>>
>>>     L148   static bool is_cleanup_needed();
>>>         nit - please move this decl above this line:
>>>
>>>         L138:   static void oops_do(OopClosure* f);
>>>
>>>         so it will be between deflate_monitor() and oops_do().
>>>         The extra blank line is not needed.
>
> Fixed
>
>>>
>>> src/share/vm/runtime/synchronizer.cpp
>>>     I like the refactoring into monitors_used_above_threshold().
>>>     No other comments.
>
> Fixed
>
>>>
>>> src/share/vm/runtime/safepoint.cpp
>>>     L528:   // Need a safepoint if there many monitor to deflate
>>>         typo - 'there many' -> 'there are many'
>>>         nit - please add a period to the end of the sentence.
>
> Fixed
>
>>
>> One more typo at L528: 'monitor' -> 'monitors'
>
> Fixed
>
>> The fix looks good to me (modulo the typos Dan pointed out).
>> I also don't need to see another webrev if the typos are fixed.
>
> Thanks, pushing!
>
> /Robbin
>
>>
>> Thanks,
>> Serguei
>>>
>>>
>>> Thumbs up! I don't need to see another webrev if you fix the
>>> typos and nits.
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Thanks for bearing with me!
>>>>
>>>> Note:
>>>> monitors_used could be wrong since we don't read the values holding 
>>>> gListLock.
>>>> So in a very unlikely scenario it could cost a safepoint.
>>>>
>>>> /Robbin
>>>>
>>>>>
>>>>> Carsten
>>>>>
>>>>> On Wed, Jun 14, 2017 at 5:40 AM, Robbin Ehn <robbin.ehn@oracle.com 
>>>>> <mailto:robbin.ehn@oracle.com>> wrote:
>>>>>
>>>>>     Hi Aleksey,
>>>>>
>>>>>     On 06/14/2017 11:14 AM, Aleksey Shipilev wrote:
>>>>>
>>>>>         On 06/14/2017 11:07 AM, Robbin Ehn wrote:
>>>>>
>>>>>             Full:
>>>>> http://cr.openjdk.java.net/~rehn/8181859/2/webrev/ 
>>>>> <http://cr.openjdk.java.net/~rehn/8181859/2/webrev/>
>>>>>
>>>>>
>>>>>         Looks good to me.
>>>>>
>>>>>
>>>>>     Thanks!
>>>>>
>>>>>
>>>>>         I would paranoidally add parentheses in this expression, 
>>>>> but your choice.
>>>>>
>>>>>         return MonitorUsedDeflationThreshold > 0 && (monitors_used 
>>>>> * 100LL) /
>>>>>         gMonitorPopulation > MonitorUsedDeflationThreshold;
>>>>>
>>>>>
>>>>>     Discussed locally and this was suggested (looks good to me):
>>>>>
>>>>>     static bool monitors_used_above_threshold() {
>>>>>        int monitors_used = gMonitorPopulation - gMonitorFreeCount;
>>>>>        int monitor_useage = (monitors_used * 100LL) / 
>>>>> gMonitorPopulation;
>>>>>        return monitor_useage > MonitorUsedDeflationThreshold;
>>>>>     }
>>>>>
>>>>>     bool ObjectSynchronizer::is_cleanup_needed() {
>>>>>        if (MonitorUsedDeflationThreshold > 0) {
>>>>>          return monitors_used_above_threshold();
>>>>>        }
>>>>>        return false;
>>>>>     }
>>>>>
>>>>>     Carsten, Aleksey, are you good with this?
>>>>>
>>>>>     Full: http://cr.openjdk.java.net/~rehn/8181859/3/webrev/ 
>>>>> <http://cr.openjdk.java.net/~rehn/8181859/3/webrev/>
>>>>>
>>>>>     Thanks, Robbin
>>>>>
>>>>>
>>>>>
>>>>>         Thanks,
>>>>>         -Aleksey
>>>>>
>>>>>
>>>>>
>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]