JDK-8256241 : replace MonitorDeflation_lock with a semaphore
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 16
  • Priority: P4
  • Status: Closed
  • Resolution: Won't Fix
  • OS: generic
  • CPU: generic
  • Submitted: 2020-11-11
  • Updated: 2021-06-24
  • Resolved: 2021-06-24
Related Reports
Relates :  
Description
[~rehn] has an idea to simplify the MonitorDeflationThread by
replacing the MonitorDeflation_lock with a semaphore.

Here's some chat notes:

Robbin Ehn  12:27
We have no critical section, there is no code which the Monitor protect. We are only using the wait/notify.
Since we can miss a notification if we are not waiting while the other thread notifies we need code to handle that.
A semaphore have no critical section, and it keep tracks of the number of notification it have receive, so we never miss a notification.
~Meaning we can remove one variable, we don't need double checking and we can remove the code needed to lock/unlock on both sides.
So a semaphore in this case have only benefits, nothing major, but simpler code. (edited) 

Daniel Daugherty  5 hours ago
Using a monitor is overkill since we don't have a critical section. Got it.
I have to think about the missing notification race.

Robbin Ehn  16:26
I now see we have not yet exposed the semaphores timedwait capability. So we can't do that :disappointed:
Assuming we do expose it:
void MonitorDeflationThread::monitor_deflation_thread_entry(JavaThread* jt, TRAPS) {
  bool was_signaled = false;
  while (true) {
    {
      while (!ObjectSynchronizer::is_async_deflation_needed()) {
        was_signaled = _sema.timedwait_with_safepoint_check(this, GuaranteedSafepointInterval);
      }
    }
    (void)ObjectSynchronizer::deflate_idle_monitors(was_signaled /* forced deflation */);
    was_signaled = false;
  }
}
(edited)

16:26
  void force_deflation() {
    _sema.signal();
  }
16:28
So instead of:
  set_is_async_deflation_requested(true);
  {
    MonitorLocker ml(MonitorDeflation_lock, Mutex::_no_safepoint_check_flag);
    ml.notify_all();
  }
You would just call:
MonitorDeflationThread::force_deflation();
16:30
But as I said it is not possible since we have not expose "timedwait_safepoint_check(...)", only wait_with_safepoint_check(...).
Comments
Runtime Triage: This is not on our current list of priorities. We will consider this feature if we receive additional customer requirements.
24-06-2021

I think we're missing the "state variable" that we're waiting for. As such there is risk of a lost notification. The semaphore would fix that bug but I still think it's the wrong tool for this situation. Plus we do a timed-wait anyway so if we did miss a notification it would only be temporary. Missed notification bugs are only a concern where there is only one notification and if we miss it then we block indefinitely.
10-05-2021

Having this as a Monitor is a typical use case in the vm and I can't imagine any performance reason to make it a semaphore. I think this should be closed as WNF.
07-05-2021

It's clear to me, but that don't mean much :) Prio 5 IMHO.
12-11-2020

A Monitor that is only used for simple event tracking is a degenerate use-case that can obviously be modelled as a semaphore instead. But to me Monitors generalize the capability to "wait for an event" by supporting timeouts and (in some cases) interruptions. I think I would rather see a Monitor used in this degenerate form than add generalized wait capabilities to Semaphore - in particular exposing a timeout requires replicating all the timeout related conversion routines. Correction: we already have the timeout conversions in the Posix version. What is missing is a timed-wait version on Windows.
11-11-2020

[~rehn] - Feel free to edit the bug to make things more clear since all I did was copy stuff from the chat.
11-11-2020