RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Jun 23 20:28:11 UTC 2020
Thanks for the fast re-review!
Dan
On 6/23/20 4:25 PM, Erik Österlund wrote:
> Hi Dan,
>
> Looks great. Ship it!
>
> Thanks,
> /Erik
>
>> On 23 Jun 2020, at 21:58, Daniel D. Daugherty <daniel.daugherty at oracle.com> 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