On Tue, 13 Aug 2024 21:45:57 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Whitespace and nits
>
> src/hotspot/share/runtime/synchronizer.cpp line 1271:
>
>> 1269: _no_progress_cnt >= NoAsyncDeflationProgressMax) {
>> 1270: double remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
>> 1271: size_t new_ceiling = ceiling / remainder + 1;
>
> Why was the `new_ceiling` calculation changed?
> I think the `new_ceiling` value is going to lower than the old ceiling value.
The old calculation made no sense if the goal was to after
NoAsyncDeflationProgressMax deflation cycles with not deflation stop trying
until we get more monitors. With the current calculation we can keep running
these no progress cycles for quite a number of iterations.
The remainder is a value in the range [0,1]. So it will always grow by at
least 1. (When remainder == 1: `new_ceiling = ceiling + 1;`)
I do however realise that the new calculation needs to handle remainder == 0.0
(MonitorUsedDeflationThreshold = 100). But the whole
NoAsyncDeflationProgressMax makes little sense if MonitorUsedDeflationThreshold
is 0, as the value for new ceiling would only stop deflation if it is large
enough that the `size_t monitor_usage = (monitors_used * 100LL) / ceiling;`
gets truncated to 0. (Less than 1%).
But this is a leftover from the deflation changes so I will probably just
remove this change and create a RFE for reevaluating this.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1716479164
