RFR(L) 8153224 Monitor deflation prolong safepoints
Karen Kinnear
karen.kinnear at oracle.com
Fri Apr 5 20:59:21 UTC 2019
Dan,
Some more minor comments from reading the code:
1. Could you add comments to markOop.hpp about
the use in the displaced_mark_word of is_marked to prevent any users of is_marked
here from needing to have that information saved/restored?
2. In objectMonitor.hpp
in is_busy you clarify the difference in use between _count (which I think you may be changing
to _contended) and _ref_count. Could you possibly also comment where you declare them?
3. clear_using_JT: would it make sense to have an assertion that _owner is either null or DEFLATER_MARKER?
4. In EnterI: if _owner == DEFLATER_MARKER there are guarantees that 0 < _count
with comments that caller ensured _count <= 0
In ReenterI: guarantee 0 <= _count, with comment not _count < 0
— Am I missing something subtle here or should they be the same guarantees?
5. I could use a little help with allocation state transitions,
e.g. in deflate_monitor_list_using_JT
you see is_new with object set so you mark it as old so next deflation will check it
- why do you set it to old here rather than in inflate once we set values?
6. Could you get rid of the new goto’s?
7. On the updated wiki for the hash race example:
Racing Threads: “T-hash is about to inc the ref_count field”
actually - T-hash just did - ref_count == 1 - so maybe change middle values
8. There is an old comment in FastHashCode
that
// WARNING:
// The displaced header is strictly immutable.
// It can NOT be changed in ANY cases.
I presume that only applies to the displaced header for a stack lock - could you
possibly update that while you are in the code?
Also in FastHashCode
// The only update to the header in the monitor (outside GC)
823 // is install the hash code. If someone add new usage of
824 // displaced header, please update this code
Can you update that comment as well? I know you’ve already updated the code logic.
So I walked the logic for the hashcode interactions - I didn’t find any holes. Thank you for walking most of it in email/wiki.
In particular, inflate does the save_om_ptr dance to inc_ref_count, so this code above will
be called while preventing async deflation.
9. install_displaced_markword_in_object
What happens if the cas_set_mark fails?
I get that today this handles the race with enter and deflate_monitor_using_JT. If we remove
the call from enter, is the expectation that we’ve blocked all others who did not set is_marked themselves?
If we remove the call from enter would it make sense to ensure that the cas_set_mark succeeds here?
10. Is there any benefit in a bit of stress testing with something like a temporary flag that deflates in
mAlloc each time it is called?
Looking forward to the performance runs as well as the latency numbers.
thanks,
Karen
> On Apr 5, 2019, at 12:10 PM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
>
> Filed:
>
> JDK-8222034 Thread-SMR functions should be updated to remove work around
> https://bugs.openjdk.java.net/browse/JDK-8222034
>
> Martin and Robbin, please check it out and make sure that I captured
> things correctly...
>
> Dan
>
>
>
> On 4/5/19 12:01 PM, Daniel D. Daugherty wrote:
>> On 4/5/19 8:37 AM, Doerr, Martin wrote:
>>> Hi everybody,
>>>
>>>> I think was fixed with:
>>>> 8202080: Introduce ordering semantics for Atomic::add/inc and other RMW atomics
>>>> You should get a leading sync and trailing one with the default conservative
>>>> model and thus get proper memory ordering.
>>>> Martin, I'm I correct?
>>> Exactly. Thanks for pointing this out. PPC uses the strongest possible ordering semantics with memory_order_conservative (default parameter).
>>> I've seen that comment about PPC in "void ThreadsList::inc_nested_handle_cnt()". This function could get replaced.
>>
>> Okay so we need a new bug to update these two Thread-SMR functions:
>>
>> src/hotspot/share/runtime/threadSMR.cpp:
>>
>> void ThreadsList::dec_nested_handle_cnt() {
>> // The decrement needs to be MO_ACQ_REL. At the moment, the Atomic::dec
>> // backend on PPC does not yet conform to these requirements. Therefore
>> // the decrement is simulated with an Atomic::sub(1, &addr).
>> // Without this MO_ACQ_REL Atomic::dec simulation, the nested SMR mechanism
>> // is not generally safe to use.
>> Atomic::sub(1, &_nested_handle_cnt);
>> }
>>
>> void ThreadsList::inc_nested_handle_cnt() {
>> // The increment needs to be MO_SEQ_CST. At the moment, the Atomic::inc
>> // backend on PPC does not yet conform to these requirements. Therefore
>> // the increment is simulated with a load phi; cas phi + 1; loop.
>> // Without this MO_SEQ_CST Atomic::inc simulation, the nested SMR mechanism
>> // is not generally safe to use.
>> intx sample = OrderAccess::load_acquire(&_nested_handle_cnt);
>> for (;;) {
>> if (Atomic::cmpxchg(sample + 1, &_nested_handle_cnt, sample) == sample) {
>> return;
>> } else {
>> sample = OrderAccess::load_acquire(&_nested_handle_cnt);
>> }
>> }
>> }
>>
>> I'll file a new bug, loop in Robbin, Erik O and Martin, and make
>> sure we're all in agreement. Once we decide that Thread-SMR's
>> functions look like, I'll adapt my Async Monitor Deflation
>> functions...
>>
>> Dan
>>
>>
>>>
>>> Best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: Robbin Ehn <robbin.ehn at oracle.com>
>>> Sent: Freitag, 5. April 2019 14:07
>>> To: daniel.daugherty at oracle.com; hotspot-runtime-dev at openjdk.java.net; Carsten Varming <varming at gmail.com>; Roman Kennke <rkennke at redhat.com>; Doerr, Martin <martin.doerr at sap.com>
>>> Subject: Re: RFR(L) 8153224 Monitor deflation prolong safepoints
>>>
>>> Hi Dan,
>>>
>>> (Martin there is question for you last in this email)
>>>
>>> After first pass I did not find any real issues.
>>> Considering what you had to work with, it looks good!
>>>
>>> #1
>>> There are some assert which are redundant (to me at least) like:
>>> src/hotspot/share/runtime/objectMonitor.cpp
>>> L445
>>> if (!dmw->is_marked() && dmw->hash() == 0) {
>>> // This dmw is neutral and has not yet started the restoration
>>> // protocol so we mark a copy of the dmw to begin the protocol.
>>> markOop marked_dmw = dmw->set_marked();
>>> assert(marked_dmw->is_marked() && marked_dmw->hash() == 0,
>>> "sanity_check: is_marked=%d, hash=" INTPTR_FORMAT,
>>> marked_dmw->is_marked(), marked_dmw->hash());
>>>
>>> That assert is basically a test that set_marked worked?
>>>
>>> L505
>>> if (Atomic::cmpxchg(Self, &_owner, DEFLATER_MARKER) == DEFLATER_MARKER) {
>>> assert(_succ != Self, "invariant");
>>> assert(_owner == Self, "invariant");
>>>
>>> Assert on _owner checks that our cmpxchg is not broken?
>>>
>>> I think it's easier to read the code if some on the most obvious asserts are
>>> removed. Maybe comments instead.
>>>
>>> #2
>>> Not your doing but I think we should remove TRAPS/Thread * Self and use
>>> JavaThread* instead.
>>> E.g. so we can change:
>>> void ObjectMonitor::EnterI(TRAPS) {
>>> Thread * const Self = THREAD;
>>> assert(Self->is_Java_thread(), "invariant");
>>> assert(((JavaThread *) Self)->thread_state() == _thread_blocked, "invariant");
>>>
>>> to:
>>>
>>> void ObjectMonitor::EnterI(JavaThread* Self) {
>>> assert(Self->thread_state() == _thread_blocked, "invariant");
>>>
>>> #3
>>> src/hotspot/share/runtime/objectMonitor.inline.hpp
>>> 164 inline void ObjectMonitor::inc_ref_count() {
>>> 165 // The increment needs to be MO_SEQ_CST. At the moment, the Atomic::inc
>>> 166 // backend on PPC does not yet conform to these requirements. Therefore
>>> 167 // the increment is simulated with a load phi; cas phi + 1; loop.
>>> 168 // Without this MO_SEQ_CST Atomic::inc simulation, AsyncDeflateIdleMonitors
>>> 169 // is not safe.
>>>
>>> I think was fixed with:
>>> 8202080: Introduce ordering semantics for Atomic::add/inc and other RMW atomics
>>> You should get a leading sync and trailing one with the default conservative
>>> model and thus get proper memory ordering.
>>> Martin, I'm I correct?
>>>
>>> Thanks, Robbin
>>>
>>> On 3/24/19 2:57 PM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> Welcome to the OpenJDK review thread for my port of Carsten's work on:
>>>>
>>>> JDK-8153224 Monitor deflation prolong safepoints
>>>> https://bugs.openjdk.java.net/browse/JDK-8153224
>>>>
>>>> Here's a link to the OpenJDK wiki that describes my port:
>>>>
>>>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation
>>>>
>>>> Here's the webrev URL:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/3-for-jdk13/
>>>>
>>>> Here's a link to Carsten's original webrev:
>>>>
>>>> http://cr.openjdk.java.net/~cvarming/monitor_deflate_conc/0/
>>>>
>>>> Earlier versions of this patch have been through several rounds of
>>>> preliminary review. Many thanks to Carsten, Coleen, Robbin, and
>>>> Roman for their preliminary code review comments. A very special
>>>> thanks to Robbin and Roman for building and testing the patch in
>>>> their own environments (including specJBB2015).
>>>>
>>>> This version of the patch has been thru Mach5 tier[1-8] testing on
>>>> Oracle's usual set of platforms. Earlier versions have been run
>>>> through my stress kit on my Linux-X64 and Solaris-X64 servers
>>>> (product, fastdebug, slowdebug).Earlier versions have run Kitchensink
>>>> for 12 hours on MacOSX, Linux-X64 and Solaris-X64 (product, fastdebug
>>>> and slowdebug). Earlier versions have run my monitor inflation stress
>>>> tests for 12 hours on MacOSX, Linux-X64 and Solaris-X64 (product,
>>>> fastdebug and slowdebug).
>>>>
>>>> All of the testing done on earlier versions will be redone on the
>>>> latest version of the patch.
>>>>
>>>> Thanks, in advance, for any questions, comments or suggestions.
>>>>
>>>> Dan
>>>>
>>>> P.S.
>>>> One subtest in gc/g1/humongousObjects/TestHumongousClassLoader.java
>>>> is currently failing in -Xcomp mode on Win* only. I've been trying
>>>> to characterize/analyze this failure for more than a week now. At
>>>> this point I'm convinced that Async Monitor Deflation is aggravating
>>>> an existing bug. However, I plan to have a better handle on that
>>>> failure before these bits are pushed to the jdk/jdk repo.
>>
>>
>
More information about the hotspot-runtime-dev
mailing list