RFR: 8332506: SIGFPE In ObjectSynchronizer::is_async_deflation_needed()

[Coleen Phillimore]

On Wed, 18 Dec 2024 15:33:43 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

> This PR solves a division by zero problem, that according to the bug report happened in `ObjectSynchronizer::is_async_deflation_needed()`. As it turns out it really happened in the inlined  `monitors_used_above_threshold()` function. The problematic line looked like this:
> 
> `size_t monitor_usage = (monitors_used * 100LL) / ceiling;`
> 
> Unfortunately the `ceiling` value was increased every time there where too many deflations without any progress. This whould eventually lead to an overflow in the `ceiling` value, and in unlucky circumstances, it would become zero. Thus causing a division by zero crash.
> 
> This PR makes sure not to increase the `ceiling` value if `monitor_usage` is below the `MonitorUsedDeflationThreshold`.
> It also makes sure the ceiling value is never zero, and does not wrap around.
> 
> Tested okay in tier1-5 and `test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java`

I have question about the math.  This is a good find and the change looks reasonable to fix the problem.  Maybe we can use this change and investigate further improvements later, but I'd like to know what you think about this first.

src/hotspot/share/runtime/synchronizer.cpp line 1289:

> 1287: 
> 1288:       if (new_ceiling < old_ceiling) {
> 1289:         new_ceiling = SIZE_MAX; // Wrap around, let's clamp new_ceiling.

Should we clamp new_ceiling to old_ceiling in this case?   With ceiling always being set to list->max() every time and list->max() never decrements, this will be true forever even if the value isn't SIZE_MAX.  I think we shouldn't use list->max() as a ceiling above and keep new_ceiling = old_ceiling in this case.  Then it seems like the thresholds will more gradually increase then have a huge increase with a lot of monitors that are later deflated.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22815#pullrequestreview-2512856914
PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1890833059