On Fri, 4 Nov 2022 12:21:22 GMT, Coleen Phillimore <cole...@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
