RFR (s): 8250606: Remove unnecessary assertions in ObjectSynchronizer FastHashcode and inflate
David Holmes
david.holmes at oracle.com
Wed Aug 5 22:31:56 UTC 2020
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");
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