RFR (s): 8250606: Remove unnecessary assertions in ObjectSynchronizer FastHashcode and inflate
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Aug 6 18:29:19 UTC 2020
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...
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