RFR: 8181859: Monitor deflation is not checked in cleanup path

[serguei.spitsyn at oracle.com]

Hi Robbin,


On 6/14/17 08:07, Daniel D. Daugherty wrote:
> On 6/14/17 7:22 AM, Robbin Ehn wrote:
>> Hi,
>>
>> On 06/14/2017 01:01 PM, Carsten Varming wrote:
>>> The last version looks good to me.
>>
>> Thanks, but unfortunately there is an error in my calculation.
>> If the is_cleanup check is run very early gMonitorPopulation is zero 
>> resulting in a division by zero.
>>
>> Incremental:
>>
>>  static bool monitors_used_above_threshold() {
>> +  if (gMonitorPopulation == 0) {
>> +    return false;
>> +  }
>>
>>
>> Full: http://cr.openjdk.java.net/~rehn/8181859/5/webrev/
>
> 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)
>
> 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.
>
> src/share/vm/runtime/synchronizer.cpp
>     I like the refactoring into monitors_used_above_threshold().
>     No other comments.
>
> 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.

One more typo at L528: 'monitor' -> 'monitors'
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,
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 at oracle.com 
>>> <mailto:robbin.ehn at 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
>>>
>>>
>>>
>