RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors
Erik Österlund
erik.osterlund at oracle.com
Tue Jun 23 13:26:25 UTC 2020
On 2020-06-23 15:05, David Holmes wrote:
> Hi Erik,
>
> On 23/06/2020 7:45 pm, Erik Österlund wrote:
>> 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.
>
> With an assert that the current thread is not the VMThread please.
Good idea David. Oh oh and Dan if you haven't looked at the review yet,
I think this could be simplified:
1316 bool ObjectSynchronizer::is_safepoint_deflation_needed() {
1317 if (!AsyncDeflateIdleMonitors) {
1318 if (monitors_used_above_threshold()) {
1319 // Too many monitors in use.
1320 return true;
1321 }
1322 return false;
1323 }
1324 return false;
1325 }
...to simply:
return !AsyncDeflateIdleMonitors && monitors_used_above_threshold()
I already got lost in the nested ifs here once.I'm okay if you don't
want to change it also.
Thanks,
/Erik
> Cheers,
> David
> -----
>
>> 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
>>>>>>
>>>>
>>>
>>
More information about the hotspot-runtime-dev
mailing list