RFR (s): 8250606: Remove unnecessary assertions in ObjectSynchronizer FastHashcode and inflate
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Aug 7 00:25:32 UTC 2020
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.
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