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

[David Holmes]

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");

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