RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors

Tue Jun 23 09:45:53 UTC 2020

Hi Dan,

Thank you for sorting this out.

I have a few thoughts about this.

1. There are seemingly two reasons why special deflation requests were 
needed.
    a) Monitors that used to get deflated before GC, now kept objects 
alive, messing with liveness assumptions of this test:
test/hotspot/jtreg/gc/g1/humongousObjects/TestHumongousClassLoader.java

    b) Tests that actually test that the monitors got deflated, as 
opposed to the associated object dying. Seemingly this test:
test/hotspot/jtreg/compiler/testlibrary/rtm/AbortProvoker.java

So my thought is that once the monitors are weak, we should not call any 
deflation logic in the TestHumongousClassLoader test. Because it should 
be expected that async deflation will not keep classes (or any other 
object) alive artificially; the test shouldn't have to know anything 
about async deflation implementation details. But we can wait with 
removing that until the weak monitors go in. We just have to remember to 
undo that part, which is okay. But we will still need the new deflation 
request mechanism for the AbortProvoker test, of course.

The other thought is that in 
ObjectSynchronizer::request_deflate_idle_monitors() we either request 
async monitor deflation or perform a forced safepoint for safepoint 
based deflation... but not if this function is called by the VM thread. 
This is the code:

1327 bool ObjectSynchronizer::request_deflate_idle_monitors() {
1328   bool is_JavaThread = Thread::current()->is_Java_thread();
1329   bool ret_code = false;
1330
1331   if (AsyncDeflateIdleMonitors) {

...

1356   } else if (!Thread::current()->is_VM_thread()) {
1357     // The VMThread only calls this at shutdown time before the final
1358     // safepoint so it should not need to force this safepoint.
1359     VM_ForceSafepoint force_safepoint_op;
1360     VMThread::execute(&force_safepoint_op);
1361     ret_code = true;
1362   }
1363
1364   return ret_code;
1365 }

And this is based on implicit knowledge about the one call from the VM 
thread (currently) being in a VM exit routine, where the safepoint based 
deflation will be performed anyway. That callsite looks like this:

  433 bool VM_Exit::doit_prologue() {
  434   if (AsyncDeflateIdleMonitors && log_is_enabled(Info, 
monitorinflation)) {
  435     // AsyncDeflateIdleMonitors does a special deflation in order
  436     // to reduce the in-use monitor population that is reported by
  437     // ObjectSynchronizer::log_in_use_monitor_details() at VM exit.
  438 ObjectSynchronizer::request_deflate_idle_monitors();
  439   }
  440   return true;
  441 }

Note that the request_deflate_idle_monitors() function is only called if 
AsyncDeflateIdleMonitors is true. And the special logic inside of 
ObjectSynchronizer::request_deflate_idle_monitors() for filtering out 
this callsite, only performs the special VM thread check when 
AsyncDeflateIdleMonitors is false. In other words: that else if path in 
request_deflate_idle_monitors could simply be else. It would be great to 
remove that special filtering so that the 
ObjectSynchronizer::request_deflate_idle_monitors() function simply does 
what it is told, without making any assumptions about who is calling the 
function and in what context.

Otherwise, this looks great!I don't need another webrev for the "else 
if" -> "else" change in request_deflate_idle_monitors().

Thanks,
/Erik

On 2020-06-22 22:22, Daniel D. Daugherty wrote:
> Greetings,
>
> Still need one more reviewer for this one. Robbin or Erik O?
> Can either of you take a look?
>
> Dan
>
>
> On 6/22/20 12:35 PM, Daniel D. Daugherty wrote:
>> Hi David,
>>
>> On 6/22/20 3:47 AM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> This all seems fine to me.
>>
>> Thanks! And thanks for the review of yet-another-monitor-subsystem fix!!
>>
>>
>>>
>>> A couple of nits:
>>>
>>> src/hotspot/share/runtime/synchronizer.cpp
>>>
>>> +     if (ret_code == false) {
>>>
>>> => if (!ret_code) {
>>
>> Nice catch. Will fix that.
>>
>>
>>> ---
>>>
>>> test/hotspot/jtreg/runtime/whitebox/TestWBDeflateIdleMonitors.java
>>>
>>>  27  * @test TestWBDeflateIdleMonitors
>>>
>>> @test is a marker. We don't/shouldn't write anything after it.
>>
>> Will fix. I got that from test/hotspot/jtreg/gc/whitebox/TestWBGC.java
>> which I copied and adapted for this new test.
>>
>>
>>>
>>> 29  * @summary Test verify that WB method deflateIdleMonitors works 
>>> correctly.
>>>
>>> "Test verify" is not grammatically correct.
>>
>> Perhaps: Test to verify that WB method deflateIdleMonitors works 
>> correctly.
>>
>> I'm sorry to say that I also got that grammatical error from
>> test/hotspot/jtreg/gc/whitebox/TestWBGC.java which I copied and adapted
>> for this new test.
>>
>> I'll file a follow-up bug for 
>> test/hotspot/jtreg/gc/whitebox/TestWBGC.java
>> so that we don't lose those fixes.
>>
>> Dan
>>
>>
>>>
>>> ---
>>>
>>> Thanks,
>>> David
>>>
>>> On 20/06/2020 2:58 am, Daniel D. Daugherty wrote:
>>>> Ping!
>>>>
>>>> And a testing update:
>>>>
>>>> Mach5 Tier[1-8] testing:
>>>>
>>>>    Tier[1-3] - done, 5 unrelated, known failures
>>>>    Tier4     - done - 1 unrelated, known failure
>>>>    Tier5     - done - no failures
>>>>    Tier6     - done, 1 unrelated, known failure
>>>>    Tier7     - almost done, 1 unrelated, known failure
>>>>    Tier8     - 56% done, 3 unrelated, known failures (so far)
>>>>
>>>> The Mach5 testing is taking longer than usual due to resource 
>>>> limitations.
>>>> So far all failures are known to be in the baseline. There have 
>>>> been no
>>>> test failures related to not deflating an idle monitor in a timely 
>>>> fashion
>>>> (so far).
>>>>
>>>> Thanks, in advance, for comments, questions or suggestions.
>>>>
>>>> Dan
>>>>
>>>> On 6/17/20 12:30 PM, Daniel D. Daugherty wrote:
>>>>> Greetings,
>>>>>
>>>>> I have a fix for cleaning up testing support for deflating idle 
>>>>> monitors.
>>>>>
>>>>> JDK-8246477 add whitebox support for deflating idle monitors
>>>>> https://bugs.openjdk.java.net/browse/JDK-8246477
>>>>>
>>>>> This project is based on jdk-16+1 and is targeted to JDK16.
>>>>>
>>>>> Here's the webrev URL:
>>>>>
>>>>> http://cr.openjdk.java.net/~dcubed/8246477-webrev/0-for-jdk16/
>>>>>
>>>>> Summary of the changes:
>>>>>
>>>>> - Add whitebox support for deflating idle monitors including
>>>>> ObjectSynchronizer::request_deflate_idle_monitors(); includes
>>>>>   a new whitebox test.
>>>>> - Drop ObjectSynchronizer::_is_special_deflation_requested flag,
>>>>>   functions and uses.
>>>>> - Switch to ObjectSynchronizer::request_deflate_idle_monitors() as 
>>>>> needed.
>>>>> - bug fix: _last_async_deflation_time_ns should be set at the end of
>>>>>   async deflation.
>>>>>
>>>>> Because this fix is removing support for special deflation requests,
>>>>> I'm doing Mach5 Tier[1-8] testing:
>>>>>
>>>>>   Tier[1-3] - almost done, 5 unrelated, known failures
>>>>>   Tier4     - done - 1 unrelated, known failure
>>>>>   Tier5     - done - no failures
>>>>>   Tier6     - almost done, 1 unrelated, known failure
>>>>>   Tier7     - almost done, 1 unrelated, known failure
>>>>>   Tier8     - > half done, 3 unrelated, known failures (so far)
>>>>>
>>>>> The Mach5 testing is taking longer than usual due to resource 
>>>>> limitations.
>>>>> So far all failures are known to be in the baseline. There have 
>>>>> been no
>>>>> test failures related to not deflating an idle monitor in a timely 
>>>>> fashion
>>>>> (so far).
>>>>>
>>>>> Thanks, in advance, for comments, questions or suggestions.
>>>>>
>>>>> Dan
>>>>
>>
>