On Mon, 14 Sep 2020 18:36:28 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> @kimbarrett
>>
>>> src/hotspot/share/runtime/objectMonitor.cpp
>>> 244 // Check that object() and set_object() are called from the right
>>> context:
>>> 245 static void check_object_context() {
>>>
>>> This seems like checking we would normally only do in a debug build. Is this
>>> really supposed to be done in product builds too? (It's written to support
>>> that, just wondering if that's really what we want.) Maybe these aren't
>>> called very often so it doesn't matter? I also see that guarantee (rather
>>> than assert) is used a fair amount in this and related code.
>>
>> I've changed check_object_context() to only be defined and called
>> when ASSERT is defined. I've also changed the guarantee() calls
>> to assert() calls.
>>
>> I've done a couple of Mach5 Tier[1-8] test cycles on this code so I'm
>> no longer worried about this code or its callers in release bits.
>
> @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.
-------------
PR: https://git.openjdk.java.net/jdk/pull/135
