RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors
David Holmes
david.holmes at oracle.com
Tue Jun 23 13:05:26 UTC 2020
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.
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