RFR: 8247281: migrate ObjectMonitor::_object to OopStorage [v2]

[Daniel D.Daugherty]

On Mon, 14 Sep 2020 18:44:30 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> @kimbarrett
>> 
>>> src/hotspot/share/runtime/synchronizer.cpp
>>> 1548 guarantee(m->object_peek() == NULL, "invariant");
>>> 
>>> Later: But see previous comment. Some of this might be relevat later though.
>>> 
>>> object_peek() seemed like the wrong operation here. I thought this was
>>> attempting to verify that the underlying WeakHandle has been released. But
>>> peeking doesn't ensure that. Oh, but we don't actually release() the
>>> WeakHandle when we "free" an OM. We're just pushing the OM on the free
>>> list. Which means the GC will continue to examine the associated OopStorage
>>> entry (and discover it's NULL). So there's some cost to not releasing the
>>> WeakHandle when putting an OM on the free list. Of course, it means there
>>> won't be any allocations when taking off the free list; I'm not sure which
>>> way is better.
>>> 
>>> But this makes me go back and wonder about whether object_peek() should have
>>> the _object.is_null() check. After creation it seems like that should never
>>> be true.
>> 
>> m->object_peek() == NULL is the right check at that location. om_release()
>> is called when we are returning an ObjectMonitor to a free list. At that point,
>> it should never be associated with an object.
>
> @kimbarrett
> 
>> src/hotspot/share/runtime/synchronizer.cpp
>> 
>> The old code in chk_in_use_entry seems wrong. It checked for a null
>> object() and recorded that as an error. But then it went on and attempted
>> to use it as if it was not null. That's been fixed by the change. However,
>> the change no longer treats a null as an error. Probably this is because
>> it's weak, and so could have become null. But is that really possible for
>> an "in use" monitor?
> 
> In the old code, an ObjectMonitor's object field should never be NULL when
> that ObjectMonitor is on an in-use list. We'll get the logging message and
> then a crash. I used to have guarantee(n->object() != NULL, ...) in there,
> but Robbin convinced me that was a waste because we'll just crash on
> the use of the NULL pointer and that was good enough.
> 
> As Erik has already explained, now that we use a weak handle, the object
> can be GC'ed before the deflater thread comes along and removes the
> idle ObjectMonitor from the in-use list.

@kimbarrett - I believe I've addressed your comments in this push:
https://github.com/openjdk/jdk/pull/135/commits/9fa2bed109d6fe352c610b26ada650226ce9cec4

-------------

PR: https://git.openjdk.java.net/jdk/pull/135