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

[Daniel D. Daugherty]

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

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