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

[Daniel D. Daugherty]

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.


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
>>
>>
>>