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

[Coleen Phillimore]

On Fri, 31 Jan 2025 04:04:54 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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?

Yes, it is useful because it removes an 'if' statement inside a lock, fixes the comments that weren't understandable and is more clear why we get here after spending time investigating what this code is doing.  So that will save time and questions later why we're calling a lock-free function inside a lock in this case and why this code is like this.

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

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