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

[Coleen Phillimore]

Deleting these asserts looks good to me.  More below.

On 8/6/20 2:29 PM, 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...

The original reason for relaxing the asserts for verification was that 
we were verifying the SystemDictionary during GC, which used 
Object.hashCode() for Klasses because they were in PermGen.  The Klass's 
prototype_header was set to never used biased locking.

Currently, the only use for calling FastHashCode() in a safepoint is 
because of dumping objects for CDS, and in this case UseBiasedLocking is 
switched off when DumpSharedSpaces.

So neither path would go through the first 'if' statement in the 
FastHashCode code.

I think you should remove the assert here.  It's obsolete and one less 
mystery to solve.

1010       // Relaxing assertion for bug 6320749.
1011       assert(Universe::verify_in_progress() ||
1012              !SafepointSynchronize::is_at_safepoint(),
1013              "biases should not be seen by VM thread here");

since revoke() asserts !at_safepoint() and change JavaThread::current() 
to "self".

Thanks,
Coleen

>
> 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
>>>>
>>>>
>>>
>