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

[Daniel D.Daugherty]

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

>> @kimbarrett
>> 
>>> src/hotspot/share/runtime/objectMonitor.cpp
>>> 249 guarantee(self->is_Java_thread() || self->is_VM_thread(), "must be");
>>> 250 if (self->is_Java_thread()) {
>>> 
>>> Maybe instead
>>> 
>>> if (self->is_Java_thread()) {
>>> ...
>>> } else {
>>> guarantee(self->is_VM_thread(), "must be");
>>> }
>> 
>> I've made this refactoring change, tweaked the comments above
>> the block a bit and switched from guarantee() to assert().
>
> @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.

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

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