RFR: 8295673: Deprecate legacy parallel class loading workaround for non-parallel-capable class loaders

[David Holmes]

On Fri, 4 Nov 2022 12:21:22 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/classfile/placeholders.cpp line 137:
>> 
>>> 135:   assert(action != PlaceholderTable::LOAD_INSTANCE || seen == NULL,
>>> 136:          "Only one LOAD_INSTANCE allowed at a time");
>>> 137: 
>> 
>> Unclear why this is removed? If disabling the new flag causes this fail then shouldn't the new flag form part of the condition?
>
> If you want, it seemed unnecessary clutter for the assert. And it's really only necessary for non null class loader data

I'd like to understand how the assert connects to the changes.

>> src/hotspot/share/classfile/systemDictionary.cpp line 597:
>> 
>>> 595:           double_lock_wait(current, lockObject);
>>> 596:         } else {
>>> 597:           return NULL;
>> 
>> Not clear why we return NULL here rather than just falling through and retrying?
>
> If we don't return we'll actually do a wait and will hang.  Worse, we'll be in a tight loop holding both the ClassLoader and SystemDictionary_lock, that the other thread can't get.

Okay but isn't that what should be expected? This is a flag to prevent a certain kind of classloader from deadlocking, so if we have that kind of classloader and the flag is disabled, then we should expect deadlock. Otherwise what will the failure mode be here? If we are introducing a new failure mode then it needs to be documented in the CSR request and possibly a Release Note.

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

PR: https://git.openjdk.org/jdk/pull/10832