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