RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Jun 24 16:46:15 UTC 2020
On 6/24/20 5:06 AM, David Holmes wrote:
> Updates look good.
Thanks for the re-review.
Dan
>
> Thanks,
> David
>
> On 24/06/2020 5:58 am, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I decided to go ahead and cleanup
>> ObjectSynchronizer::is_safepoint_deflation_needed()
>> in addition to the other changes.
>>
>> I've made the following changes since the original webrev:
>>
>> - addressed David H's code review comments
>> - addressed Erik O's code review comments
>> - rebased to jdk-16+2
>>
>> I'm currently running a Mach5 Tier[1-3] to verify.
>>
>> Full CR1 webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8246477-webrev/1-for-jdk16.full/
>>
>> Incremental CR1 webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8246477-webrev/1-for-jdk16.inc/
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>
>>
>> On 6/23/20 10:24 AM, Erik Österlund wrote:
>>> 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