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

David Holmes david.holmes at oracle.com
Fri Aug 7 00:49:57 UTC 2020


On 7/08/2020 10:25 am, Coleen Phillimore wrote:
> On 8/6/20 7:49 PM, David Holmes wrote:
>> On 7/08/2020 9:37 am, Ioi Lam wrote:
>>>
>>>
>>> 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;
>>
>> Ah! :(
>>
>>> 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 ()
>>
>> Know bug: JDK-8251118
>>
>>> #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.
>>
>> There was some feedback on the disabling that indicated it may still 
>> be useful so that decision to proceed with the removal has yet to occur.
>>
>>> Maybe instead of removing the asserts, just enclose them inside "if 
>>> UseBiasedLocking"?
>>
>> I think I'm going to fix the BL block of code separately. There's no 
>> reason I can see why a biased-locked object should be a concern in 
>> FastHashCode, we just need to change the revocation code to allow for 
>> the safepoint (which is exactly what we do elsewhere).
> 
> I'm not sure why you want to do this, since I can't see why anything 
> would call this with BiasedLocking on.  But for what it's worth, I had a 
> prototype of something else where I changed that code to be:
> 
>    if (SafepointSynchronize::is_at_safepoint()) {
>      BiasedLocking::revoke_at_safepoint();
>    } else {
>      BiasedLocking::revoke();
>    }
> 
> and it worked fine.

Yes that is exactly what I had in mind. If nothing else it avoids having 
to think about why the BL path can't happen in a safepoint. Also it may 
be that once this is fixed the CDS code may not have to disable BL.

Thanks,
David

> Coleen
> 
>>
>> Thanks,
>> David
>>
>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
> 


More information about the hotspot-runtime-dev mailing list