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

[Ioi Lam]

On 8/6/20 2:46 PM, David Holmes wrote:
> 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
> -----
>

Oh I feel so dumb. I forgot that we force UseBiasedLocking to false when 
CDS is enabled (arguments.cpp)!

   if (DumpSharedSpaces) {
     // Disable biased locking now as it interferes with the clean up of
     // the archived Klasses and Java string objects (at dump time only).
     UseBiasedLocking = false;

Anyway, it seems like UseBiasedLocking is not compatible with CDS in 
many ways. I have updated the patch to disable the above line. I also 
use a more reliably way get a biased-locked object. The test locks on an 
interned string, which we know for sure no one else would lock, or 
compute identity_hash of it.

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

and when dumping with -XX:+UseBiasedLocking, we crash while doing GC 
inside a safepoint:

#19 0x00007ffff5bb1a80 in oopDesc::set_mark (this=0xbcbcbcbcbcbcbcbc, m=...)
#20 0x00007ffff5bb0d0b in BiasedLocking::restore_marks ()
#21 0x00007ffff5fa3515 in G1FullCollector::complete_collection (...)
#22 0x00007ffff5f68b57 in G1CollectedHeap::do_full_collection (...)
#23 0x00007ffff5f68bac in G1CollectedHeap::do_full_collection (...)
#24 0x00007ffff5dc7aad in CollectedHeap::collect_as_vm_thread (...)
#25 0x00007ffff60885f4 in HeapShared::run_full_gc_in_vm_thread ()

So it looks like the G1GC doesn't support collection inside the VM 
thread with biased-locked objects.

Is biased locking going to be removed soon? I think we probably 
shouldn't spend too much time on it.

Maybe instead of removing the asserts, just enclose them inside "if 
UseBiasedLocking"?

Thanks
- Ioi


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