RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors
Erik Österlund
erik.osterlund at oracle.com
Tue Jun 23 14:24:17 UTC 2020
Hi Dan,
On 2020-06-23 16:20, Daniel D. Daugherty wrote:
> On 6/23/20 9:26 AM, Erik Österlund wrote:
>>
>>
>> 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.
>
> As I said to David. It's okay if the VMThread executes the VM-op, but
> it is just not necessary.
Okay.
>
>> 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.
>
> I could clean it up, but since I'm about to delete that function...
Deletion sounds better. You can skip it for now, and let's delete it
soon instead. :)
/Erik
> Dan
>
>
>>
>> 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