RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors
David Holmes
david.holmes at oracle.com
Wed Jun 24 09:06:00 UTC 2020
Updates look good.
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