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

[Coleen Phillimore]

On Wed, 18 Dec 2024 22:24:29 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

>> 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 rather than have a huge increase with a lot of monitors that are later deflated.
>
> @coleenp 
> If we start with your "I have question about the math". Well, so do I. I don't understand the math on line 1286 that calculates `new_ceiling`. I would expect that if `monitor_usage` is above the `MonitorUsedDeflationThreshold` the `new_ceiling` value should be set high enough so that `monitor_usage` will become lower than `MonitorUsedDeflationThreshold` next time. But looking at the trace output from `MonitorUsedDeflationThresholdTest.java` this is not the case. Instead it increases more slowly until it reaches a value where the `monitor_usage` is below the threshold. It's hard to call that a bug, but it doesn't seem like the optimal way of doing it.
> 
> What value to use for clamping `new_ceiling` is probably only an academic question, since I now only increase the ceiling value when the `monitor_usage` really is above the threshold. This basically means that we will not see any wrap around. I only added the wrap around code (clamp code) to show that ceiling can never be zero and thus the bug I'm fixing is gone.
> 
> Discussed the math stuff with @fisk in the office today. We concluded that this PR fixes the divide by zero problem which has been in the code since 2021, and it would be nice to have that fixed in the 24 release. Then we can create a new feature request to fix more optimal math stuff in time for the 25 release.

Okay, I was wondering what you'd found from tracing the ceiling code. I'm glad it increases more slowly with your patch.  The value of SIZE_MAX is okay for now, and we can go back to this code and figure out if all this math is helpful for performance or not.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1891886498