RFR (s): 8250606: Remove unnecessary assertions in ObjectSynchronizer FastHashcode and inflate

[David Holmes]

Hi Dan,

On 7/08/2020 4:29 am, Daniel D. Daugherty wrote:
> On 8/5/20 6:31 PM, David Holmes wrote:
>> Hi Dan,
>>
>> Thanks for taking a look at this.
>>
>> On 6/08/2020 4:08 am, Daniel D. Daugherty wrote:
>>> On 8/5/20 3:58 AM, David Holmes wrote:
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8250606
>>>> webrev: http://cr.openjdk.java.net/~dholmes/8250606/webrev/
>>>
>>> src/hotspot/share/runtime/synchronizer.cpp
>>>      L1010:       // Relaxing assertion for bug 6320749.
>>>      L1011:       assert(Universe::verify_in_progress() ||
>>>      L1012: !SafepointSynchronize::is_at_safepoint(),
>>>      L1013:              "biases should not be seen by VM thread here");
>>>          This assertion is similar and tagged with the same bug ID 
>>> (6320749)
>>>          as the ones that you removed. Granted that the Biased 
>>> Locking code
>>>          is on the way out, but...
>>>
>>>          I'm fine if you plan to leave this one for Biased Locking 
>>> removal.
>>
>> Yeah I confess I had my biased-locking filter on when looking at this 
>> part of the code. You've actually exposed a bit of a mess. :)
>>
>> It is unclear why the same !at_safepoint assertion was initially added 
>> to the biased-locking revocation chunk of code given the same 
>> assertion would then be executed after it. But I think it is more to 
>> do with the line following the assertion (original code):
>>
>> assert(!SafepointSynchronize::is_at_safepoint(), "biases should not be 
>> seen by VM thread here");
>> BiasedLocking::revoke_and_rebias(hobj, false, JavaThread::current());
>>
>> If we are at a safepoint in the VMThread then JavaThread::current() 
>> will assert! So even after the assertion was apparently relaxed with 
>> the verify_in_progress() check, we could never have hit this code as 
>> the JavaThread::current() would still assert. So I can (and should) 
>> remove the assertion here as well (CDS dump could reach this in 
>> theory), but I also need to correct this current code:
>>
>> -      BiasedLocking::revoke(hobj, JavaThread::current());
>> +      BiasedLocking::revoke(hobj, self);
>>
>> as re-manifesting the current thread is pointless. That said the above 
>> code (nor the original) also can't execute at a safepoint:
>>
>> void BiasedLocking::revoke(Handle obj, TRAPS) {
>>   assert(!SafepointSynchronize::is_at_safepoint(), "must not be called 
>> while at safepoint");
> 
> 
> With the assert in JavaThread::current() and with the assert in
> BiasedLocking::revoke(), I don't see how we could ever get away
> with calling FastHashCode() on a biased object at a safepoint.
> It simply should have blown up (with non-release bits).
> 
> I'm not yet sure what keeps us from seeing a biased object at
> a safepoint, but there has to be something there. We aren't that
> lucky to have simply missed this...

Seems we are that lucky in a sense. It seems that the verification 
process that we allow for is not the same as it once was and so we don't 
hit the code that needs the FastHashCode. The only other safepoint op 
that gets involved is CDS dumping and there we also get lucky in that 
none of the objects that get archived are bias-locked. I've been using 
Ioi's test from

http://cr.openjdk.java.net/~iklam/jdk16/8249276-reset-archive-obj-headers.v02/

to force the object to be biased but I still can't hit that code 
fragment so something else is still in play.

David
-----

> Dan
> 
>>
>> so even larger changes will be needed to make this code work at a 
>> safepoint, and that is going well beyond the intended scope of this 
>> particular bug. :( I'll need to get back to you.
>>
>> I also need to check our GC verify tests to see how we manage to 
>> apparently never encounter a biased-locked object, otherwise we would 
>> have seen this inconsistency in the code.
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> Thumbs up.  Thanks for digging up the history behind these very old 
>>> asserts.
>>
>> Thanks,
>> David
>>
>>> Dan
>>>
>>>
>>>>
>>>> tl;dr I removed some assertions that were deemed unnecessary after 
>>>> interfering with other work. :)
>>>>
>>>> ObjectSynchronizer::inflate had the following assertion added back 
>>>> in JDK 6 under JDK-5030359:
>>>>
>>>> ObjectMonitor * ObjectSynchronizer::inflate (oop object) {
>>>>   // Inflate mutates the heap ...
>>>>   assert (!SafepointSynchronize::is_at_safepoint(), "invariant") ;
>>>>
>>>> That same change added FastHashcode which had the following assertions:
>>>>
>>>> intptr_t ObjectSynchronizer::FastHashCode (Thread * Self, oop obj) {
>>>>   assert (!SafepointSynchronize::is_at_safepoint(), "invariant") ;
>>>>   assert (Self->is_Java_thread(), "invariant") ;
>>>>   assert (((JavaThread *)Self)->thread_state() != _thread_blocked, 
>>>> "invariant") ;
>>>>
>>>> These methods had assertions that they were not executed at a 
>>>> safepoint in case the modification of the markWord could cause 
>>>> interference with GC.
>>>>
>>>> JDK-6320749 then relaxed the assertions to allow for GC verification 
>>>> - which does happen at a safepoint and so obviously not in a 
>>>> JavaThread.
>>>>
>>>> The FastHashCode assertions were further relaxed in JDK-8015086 to 
>>>> allow the CDS dump VM safepoint operation to call it. But 
>>>> FastHashCode can also call inflate so that was a mismatch in 
>>>> assertions just waiting to happen - which is how I hit this in a VM 
>>>> that always forces inflation :)
>>>>
>>>> I initially intended to extend the FastHashCode assertion to match 
>>>> inflate, but then a question arose about the safety of the CDS 
>>>> dumping code using FastHashcode which showed that the code is in 
>>>> fact safe. Notwithstanding what the state of the world may have been 
>>>> in the JDK 6 timeframe, there is now no potential interference 
>>>> possible between execution of these methods at an arbitrary 
>>>> safepoint and any concurrent GC use of the markWord. So the original 
>>>> !is_at_safepoint assertion can just be dropped from both methods. 
>>>> That in turn means we either drop the JavaThread assertion or else 
>>>> expand it to be "JavaThread or VMThread" - but that itself seems 
>>>> unnecessarily restrictive: why can't a GC thread use this code for 
>>>> example? So I chose to drop it.
>>>>
>>>> That leaves the "if I'm a JavaThread I'm not _thread_blocked" 
>>>> assertion. I don't see that this really guards against anything 
>>>> useful (a JavaThread should be _thread_in_vm when executing this 
>>>> code ... though there may be leaf calls where the thread is 
>>>> _thread_in_java). So again I just dropped the assertion.
>>>>
>>>> So a long winded justification for dropping a few assertions. :)
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>
>