RFR: 8277631: ZGC: CriticalMetaspaceAllocation asserts [v2]

[David Holmes]

On Wed, 24 Nov 2021 08:38:16 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> src/hotspot/share/memory/metaspaceCriticalAllocation.cpp line 130:
>> 
>>> 128: void MetaspaceCriticalAllocation::wait_for_purge(MetadataAllocationRequest* request) {
>>> 129:   for (;;) {
>>> 130:     ThreadBlockInVM tbivm(JavaThread::current());
>> 
>> Can't you move the TBIVM outside of the loop now that it is always created?
>
>> Just a comment but it always concerns me that if we have to manually add a TBIVM when using a non-safepoint-checking lock then the lock is mis-classified as a non-safepoint-checking one! :(
>> 
>> That aside changes look fine. A few grammatical nits in the test.
>> 
>> Thanks, David
> 
> Thanks for the review David. I fixed your nits.
> 
> I agree that this lock should preferably have been a safepoint checking lock. In fact, it *was* a safepoint checking lock when I wrote the code. But that was before we changed the locking rules so that whether we do safepoint checking or not is a function of the rank. After that, this lock has become constrained to the current low rank by the current set of other locks, and by being that low ish rank, it is not allowed to safepoint check, unless I move a bunch of other lock ranks around, and figure out if it's okay for those locks to start safepoint checking as well, which isn't entirely obvious.
> 
> So this lock might be a case where those new rules end up being a bit awkward, and have lead this code to do manual transitions to blocked instead, as an escape hatch from the asserts.

Ah! I forgot about the rank changes that forced this dichotomy.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6520