RFR: 8332506: SIGFPE In ObjectSynchronizer::is_async_deflation_needed() [v3]

[David Holmes]

FYI for the mailing list records, this comment was deleted in the PR.

David

On 7/01/2025 10:56 am, David Holmes wrote:
> On Tue, 7 Jan 2025 00:43:59 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> If `list->max()` is above `old_ceiling` we should use that because the number of actual used monitors, has at one point exceeded the `old_ceiling` value. Why add `monitors_used` to the mix? Any increase in the `list->max()` value will come  **after** the increase in `list->count()` aka `monitors_used`. So by adding `monitors_used` to the mix we ensure that `ceiling` can't become zero, even if `old_ceiling` is zero and `list->max()` hasn't been increased yet. Because we [know](https://github.com/openjdk/jdk/blob/502766b759d13edd1ce3f4ebd46fcc8f13ed5d53/src/hotspot/share/runtime/synchronizer.cpp#L1268) that `monitors_used` is not zero.
>>>
>>> I tried to condense that into a shorter comment, but obviously I failed.
>>
>> But `monitors_used == list->count()` and `list->count()` by definition must be <= `list->max()`, so including `monitors_used` in the `MAX3` expression is pointless.
> 
> Are you worried about the 0 to 1 transition of `list->count()` i.e that we may be racing with this code:
> 
> void MonitorList::add(ObjectMonitor* m) {
>    ObjectMonitor* head;
>    do {
>      head = Atomic::load(&_head);
>      m->set_next_om(head);
>    } while (Atomic::cmpxchg(&_head, head, m) != head);
> 
>    size_t count = Atomic::add(&_count, 1u);
>    if (count > max()) {
>      Atomic::inc(&_max);
>    }
> }
> 
> ? Surely that cannot be allowed to happen. But lets assume it can then it would be much clearer IMO to have:
> 
>   size_t ceiling = MAX3(old_ceiling, list->max(), 1);
> 
> to exclude the zero case.
> 
> -------------
> 
> PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1904783614