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

[Daniel D. Daugherty]

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.

Thumbs up.  Thanks for digging up the history behind these very old asserts.

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