RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Jun 23 14:20:32 UTC 2020
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.
> 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...
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