Async Monitor Deflation design review (2019.04.02)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Apr 15 17:13:37 UTC 2019
Just tracking to make sure I made all the changes...
I filed:
JDK-8222295 more baseline cleanups from Async Monitor Deflation project
https://bugs.openjdk.java.net/browse/JDK-8222295
to track this round of baseline cleanups.
On 4/5/19 9:48 AM, Daniel D. Daugherty wrote
> On 4/3/19 5:12 PM, Karen Kinnear wrote:
>
>
> src/hotspot/share/runtime/objectMonitor.hpp:
>
>> volatile jint _count; // reference count to prevent
>> reclamation/deflation
>> // at stop-the-world time. See
>> ObjectSynchronizer::deflate_monitor().
>> // _count is approximately
>> |_WaitSet| + |_EntryList|
>
> The above comment in objectMonitor.hpp is from the base system and it
> is partially misleading and partially wrong. This comment should be
> updated in the base system to something like this:
>
> // Number of active contentions in enter(). It is used by is_busy()
> // along with other fields to determine if an ObjectMonitor can be
> // deflated. See ObjectSynchronizer::deflate_monitor().
Done via 8222295.
> This part: "_count is approximately |_WaitSet| + |_EntryList|" is wrong
> since _WaitSet is associated with Wait and Notify/NotifyAll operations
> and not Enter operations.
>
> I'm also thinking of renaming '_count' -> '_contentions'. See below.
>
>
>> static int count_offset_in_bytes() { return
>> offset_of(ObjectMonitor, _count); }
>
> This function appears to be unused and should be removed.
Done via 8222295.
> src/hotspot/share/runtime/objectMonitor.inline.hpp
>
>> inline jint ObjectMonitor::count() const {
>> return _count;
>> }
>
> The count() getter appears to be unused and should be deleted.
Done via 8222295. There was also a 'set_count()' declared in
objectMonitor.hpp, but never defined anywhere so I also removed it.
>
>> if (Atomic::cmpxchg(DEFLATER_MARKER, &mid->_owner, (void*)NULL) ==
>> NULL) {
>
> Hmmm... that code pre-dates Atomic::replace_if_null()... should
> switch to that...
Done.
>
>> Good to add comments/assertions/guarantees
>> to future proof.
>
> I agree. I'll look at adding comments and guarantee() calls to cover
> our assumptions when the object's header/dmw is stored into the
> ObjectMonitor's _header field.
Here's the existing code that saves the 'dmw' into ObjectMonitor::_header:
> markOop dmw = mark->displaced_mark_helper();
> assert(dmw->is_neutral(), "invariant");
>
> // Setup monitor fields to proper values -- prepare the monitor
> m->set_header(dmw);
is_neutral() is defined as:
> bool is_neutral() const { return (mask_bits(value(),
biased_lock_mask_in_place) == unlocked_value); }
is_marked() is defined as:
bool is_marked() const {
return (mask_bits(value(), lock_mask_in_place) == marked_value);
}
and:
> lock_bits = 2,
> enum { lock_mask = right_n_bits(lock_bits),
> lock_mask_in_place = lock_mask << lock_shift,
> biased_lock_bits = 1,
> biased_lock_mask = right_n_bits(lock_bits +
biased_lock_bits),
> biased_lock_mask_in_place= biased_lock_mask << lock_shift,
> unlocked_value = 1,
> marked_value = 3,
So the location of the mark bits (2 bits) overlaps with the location of
biased_lock_mask (3 bits) which means that the is_neutral() check will
catch if the mark bits are set:
marked_value != unlocked_value
So here's the change for the code path that converts the stack
lock into an inflated ObjectMonitor::
- assert(dmw->is_neutral(), "invariant");
+ // Catch if the object's header is not neutral (not locked and
+ // not marked is what we care about here).
+ assert(dmw->is_neutral(), "invariant: header=" INTPTR_FORMAT,
p2i((address)dmw));
Same assert, but better diagnostics.
So here's the change for the code path that converts a neutral header
into an inflated ObjectMonitor:
- assert(mark->is_neutral(), "invariant");
+ // Catch if the object's header is not neutral (not locked and
+ // not marked is what we care about here).
+ assert(mark->is_neutral(), "invariant: header=" INTPTR_FORMAT,
p2i((address)mark));
Same assert, but better diagnostics.
Done via 8222295.
For the Async Monitor Deflation project, we'll change those from
assert() to guarantee() for now.
> 453 // There can be three different racers trying to update the _header
> 454 // field and the return dmw value will tell us what cleanup needs
> 455 // to be done (if any) after the race winner:
> 456 // 1) A mutator trying to install a hash in the object.
> 457 // Note: That mutator is not executing this code, but it is
> 458 // trying to update the _header field.
> 459 // If winner: dmw will contain the hash and be unmarked
>
> I no longer think my port has race #1 due to the ref_count changes.
> I have to go back and verify the location of that race in Carsten's
> port and then verify my port again.
Here's Carsten's code that handled the race between T-deflate and T-hash
(L813-822 and L830-4):
> 803 // Inflate the monitor to set hash code
> 804 monitor = ObjectSynchronizer::inflate(Self, obj, inflate_cause_hash_code);
> 805 // Load displaced header and check it has hash code
> 806 mark = monitor->header();
> 807 assert(mark->is_neutral() || mark->hash() == 0 &&
> mark->is_marked(), "invariant");
> 808 hash = mark->hash();
> 809 if (hash == 0) {
> 810 hash = get_next_hash(Self, obj);
> 811 temp = mark->set_unmarked()->copy_set_hash(hash); // merge hash
> code into header
> 812 assert(temp->is_neutral(), "invariant");
> 813 if (mark->is_marked()) {
> 814 // Monitor is being deflated. Try installing mark word with hash
> code into obj.
> 815 markOop monitor_mark = markOopDesc::encode(monitor);
> 816 if (obj->cas_set_mark(temp, monitor_mark) == monitor_mark) {
> 817 return hash;
> 818 } else {
> 819 // Somebody else installed a new mark word in obj. Start over. We
> are making progress,
> 820 // as the new mark word is not a pointer to monitor.
> 821 goto Retry;
> 822 }
> 823 }
> 824 test = (markOop) Atomic::cmpxchg_ptr(temp, monitor, mark);
> 825 if (test != mark) {
> 826 // The only update to the header in the monitor (outside GC) is
> install
> 827 // the hash code or mark the header to signal that the monitor is
> being
> 828 // deflated. If someone add new usage of displaced header, please
> update
> 829 // this code.
> 830 if (test->is_marked()) {
> 831 // Monitor is being deflated. Make progress by starting over.
> 832 assert(test->hash() == 0, "invariant");
> 833 goto Retry;
> 834 }
> 835 hash = test->hash();
> 836 assert(test->is_neutral(), "invariant");
> 837 assert(hash != 0, "Trivial unexpected object/monitor header usage.");
> 838 }
> 839 }
> 840 // We finally get the hash
> 841 return hash;
> 842 }
And in my port, the same race is here (L810):
> 808 // Inflate the monitor to set hash code
> 809 ObjectMonitorHandle omh;
> 810 inflate(&omh, Self, obj, inflate_cause_hash_code);
> 811 monitor = omh.om_ptr();
> 812 // Load displaced header and check it has hash code
> 813 mark = monitor->header();
> 814 assert(mark->is_neutral(), "invariant: header=" INTPTR_FORMAT, p2i((address)mark));
> 815 hash = mark->hash();
> 816 if (hash == 0) {
> 817 hash = get_next_hash(Self, obj);
> 818 temp = mark->copy_set_hash(hash); // merge hash code into header
> 819 assert(temp->is_neutral(), "invariant: header=" INTPTR_FORMAT, p2i((address)temp));
> 820 test = Atomic::cmpxchg(temp, monitor->header_addr(), mark);
> 821 if (test != mark) {
> 822 // 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
> 825 hash = test->hash();
> 826 assert(test->is_neutral(), "invariant: header=" INTPTR_FORMAT, p2i((address)test));
> 827 assert(hash != 0, "Trivial unexpected object/monitor header usage.");
> 828 }
> 829 }
> 830 // We finally get the hash
> 831 return hash;
> 832 }
All of the racing in my port is done inside inflate() with the call
to save_om_ptr() so the remainder of the hashcode update is back to
the baseline code.
> 480 // Note: If a mutator won the cmpxchg() race above and installed
> a hash
> 481 // in _header, then the updated dmw contains that hash and
> we'll install
> 482 // it in the object's markword here.
>
> The comment on L480-481 does not quite make sense. A mutator trying
> to install a hashcode does not call this function and if we already
> have a hashcode in the initial 'dmw', then we won't call cmpxchg()
> above.
>
> However, it is possible for our two callers to
> install_displaced_markword_in_object() to have two different
> initial 'dmw' values for the same object. For example:
>
> - T-deflate can have a 'dmw' without a hashcode.
> - T-enter can have a 'dmw' with a hashcode if a hashcode was saved in
> the ObjectMonitor's header/dmw after T-deflate grabbed its initial
> 'dmw' value and before T-enter grabbed its initial 'dmw' value.
>
> Because T-deflate's initial 'dmw' does not have a hashcode, it will
> go thru the restoration protocol but this:
>
> > 464 dmw = (markOop) Atomic::cmpxchg(marked_dmw, &_header, dmw);
>
> will not update the ObjectMonitor's header/dmw field because T-deflate's
> initial 'dmw' value no longer matches the ObjectMonitor's current
> header/dmw field which now has a hashcode. The return value from
> cmpxchg() will be the ObjectMonitor's current header/dmw value including
> the hashcode so both T-deflate and T-enter will be racing to set the
> object's header to the 'dmw' with the hashcode.
>
>
> 483 obj->cas_set_mark(dmw, markOopDesc::encode(this));
> 484 }
>
>
> So to make this incredibly long hashcode story short:
>
> If a hashcode was set in the ObjectMonitor's header/dmw field, then
> it will be restored to the object's header by either T-enter's or
> T-deflate's install_displaced_markword_in_object()
>
>
> Side note: I could do a set of diagrams for two different calls to
> install_displaced_markword_in_object() where T-enter has the newly set
> hashcode and T-deflate does not have the hashcode to show that it will
> resolve right. However, if we drop install_displaced_markword_in_object()
> from T-enter as mentioned below, then we don't have to do that because
> we no longer have that race.
The comment on L480-2 needs tweaking, but I don't think the scenario
that I posited above can actually happen.
So I made a complete pass through install_displaced_markword_in_object()
and updated comments and assertions. Pretty much a complete rewrite of
comments and assertions so it will be need to re-reviewed carefully.
Okay, I believe I've made all the changes for this set of comments
and replies...
Dan
More information about the hotspot-runtime-dev
mailing list