RFR: 8337548: Parallel class loading can pass is_superclass true for interfaces [v2]

[David Holmes]

On Thu, 30 Jan 2025 14:22:06 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/classfile/systemDictionary.cpp line 509:
>> 
>>> 507:                                                                        superclassname,
>>> 508:                                                                        class_loader,
>>> 509:                                                                        false,
>> 
>> So this is really the only actual change intended and the rest is just code reshuffling?
>> 
>> The reshuffle seems okay but I always see a red flag when anything gets moved outside a locked region of code. I am concerned that the super check and the check of the placeholder are no longer atomic. What happens if after we have checked the superclass and not found it, that another thread loads the current class (and/or superclass) before we acquire the SD lock to check for the placeholder?
>
> Most of the change was the code reshuffling noting that find_class doesn't require the SystemDictionary lock and isn't required if we're not checking the super class of the class we're trying to load.
> 
> If we find the current class outside the lock, there will be Klass->_super assigned since we will never insert a partially resolved class in the SystemDictionary.   If the super->name doesn't match the one we're trying to resolve, it's probably an error now that I think about it if is_superclass is only called for the one case where we're parsing the super class.  I don't actually know how to write this or where the error would occur, but I will try.
> 
> I think your question boils down to the restructuring the find_class outside the lock (forget the super check in the if statement):
> A Lock; find_class; if not found create placeholder(current, super); Unlock B
> vs.
> find_class; A if not found create placeholder(current, super); Unlock B
> 
> The SystemDictionary_locked regions are short which makes them all need analysis.  If the class is resolved at A, we won't create the placeholder in case 1, but we would in case 2.  In case 2, we'd then call resolve_instance_class_or_null which would find the class and return, remove the placeholder, and just do something quickly but perhaps unnecessary.  But from testing, since we are resolving from stream when we call this, we never have the class already resolved at this point even by another thread (except for redefinition).  The other thread should normally be waiting at the class-loader lock or the DEFINE_CLASS placeholder.  Maybe it's possible to construct a scenario that this is possible.
> 
> The other case where we might get to this code is handle_parallel_super_load - another thread has created the LOAD_SUPER/CHECK_CIRCULARITY placeholder while also loading the current class and this thread has to check for circularity also because in the parallelCapable class loader case, it may be holding the superclass name lock.  Can the first thread complete loading, causing the second to unnecessarily create a CHECK_CIRCULARITY placeholder?  Maybe, I don't see how, but I don't think it's harmful since the second thread will do a resolve_instance_class_or_null and return the class and remove the placeholder.
> 
> I thought of taking this code out until I found redefinition does find this case because the class is already loaded, but finding invariants to simplify this logic is always the goal.

That all sounds reasonable but without knowing the exact detailed code paths involved I find it hard to assess the risk here. Does the locking refactor actually buy us anything useful?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23322#discussion_r1936650374