RFR(S)[16]: 8246477: add whitebox support for deflating idle monitors
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Jun 23 14:14:27 UTC 2020
Hi Erik,
Thanks for the review...
On 6/23/20 5:45 AM, Erik Österlund wrote:
> Hi Dan,
>
> Thank you for sorting this out.
You're welcome.
> 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
Minor clarification: that library code is used by two RTM tests.
> 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.
We still haven't finished discussing the details of making monitor
linkages weak, but we'll get there. The devil is in the details
with doing the transition to weak linkages...
Part of our due diligence for that work will be to revisit the
calls to WhiteBox.deflateIdleMonitors().
> 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.
A clarification here:
- VM_Exit::doit_prologue() is not called by the VMThread.
It is executed by the JavaThread that requested the VM_Exit VM-op.
The call site that is invoked by the VMThread is:
src/hotspot/share/runtime/vmThread.cpp:
L251: void VMThread::run() {
<snip>
L287: if (AsyncDeflateIdleMonitors && log_is_enabled(Info,
monitorinflation)) {
L288: // AsyncDeflateIdleMonitors does a special deflation in order
L289: // to reduce the in-use monitor population that is
reported by
L290: // ObjectSynchronizer::log_in_use_monitor_details() at VM
exit.
L291: ObjectSynchronizer::request_deflate_idle_monitors();
L292: }
However, the same point applies since this call to
request_deflate_idle_monitors()
is also guarded by "if (AsyncDeflateIdleMonitors ...".
> 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.
This bug fix: 8246477: add whitebox support for deflating idle monitors
is a sub-task of this bug:
JDK-8246476 remove AsyncDeflateIdleMonitors option and the
safepoint based deflation mechanism
https://bugs.openjdk.java.net/browse/JDK-8246476
so the plan is to remove this entire block (soon, very, very soon):
> 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 }
along with the rest of the safepoint based deflation mechanism.
Yes, for the current code in the webrev, the else part of L1356 is not
needed. No argument.
So why is it this way? I have included calls to
request_deflate_idle_monitors()
in one of my debugging kits to clean things up before dumping the current
info about all the in-use ObjectMonitors. By having
request_deflate_idle_monitors()
not do a VM_ForceSafepoint VM-op when called by the VMThread, I could more
easily sprinkle these bits of debugging code without unexpected behaviors
when running with !AsyncDeflateIdleMonitors.
However, these debugging kits are less relevant now that the main bug
(8153224) has been pushed!
> Otherwise, this looks great!I
Thanks! I thought you would appreciate the special deflation request
mechanism going away...
> don't need another webrev for the "else if" -> "else" change in
> request_deflate_idle_monitors().
I'll make that change.
Thanks again for the review.
Dan
>
> 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