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

[Robbin Ehn]

Hi all,

Thanks for the reviews!

On 06/14/2017 10:19 PM, serguei.spitsyn at 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 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
>>>>
>>>>
>>>>
>>
>