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

[Coleen Phillimore]

On Fri, 31 Jan 2025 14:06:36 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/classfile/systemDictionary.cpp line 433:
>> 
>>> 431:   // Also rarely, if parallel loading completes loading at this point, we can also check the super class name.
>>> 432:   if (check_is_superclass) {
>>> 433:     InstanceKlass* klassk = loader_data->dictionary()->find_class(THREAD, class_name);
>> 
>> Maybe the comment should be moved like this. That will give it better context.
>> 
>> 
>>  if (check_is_superclass) {
>>     InstanceKlass* klassk = loader_data->dictionary()->find_class(THREAD, class_name);
>>     if (klassk != nullptr) {
>>       // We can come here for two reasons:
>>       // (a) RedefineClasses -- the class is already loaded
>>       // (b) Rarely, the class might have been loaded by a parallel thread
>>       // We can do a quick check against the already assigned superclass's name and loader.
>>       if (((quicksuperk = klassk->java_super()) != nullptr) &&
>>           ((quicksuperk->name() == next_name) &&
>>           (quicksuperk->class_loader() == class_loader()))) {
>>         return quicksuperk;
>>       }
>> 
>> 
>> Also, I am not sure `check_is_superclass` is a better name than `is_superclass`. When you have the chain:
>> 
>> 
>> resolve_super_or_fail(Symbol* class_name, Symbol* super_name, ...   bool is_superclass, ...)
>> -> resolve_with_circularity_detection(class_name, super_name, class_loader, is_superclass, THREAD)
>> 
>> 
>> The caller of `resolve_super_or_fail()` only knows that `super_name` is an (immediate) superclass of `class_name`. It doesn't know (or need to care) that someone down the call chain would have to do some "check" in for optimization purposes.
>> 
>> I think it's fine to move this quick check outside of the lock. I don't think we should worry about removing locks that are obviously not needed. If we somehow depended on the lock always being taken, that would be a bug elsewhere and we will find out during testing.
>
> Okay, I renamed is_superclass to check_is_superclass when I the code wasn't sure if it was the superclass, but now it's only true when it's sure it's the superclass.  So now is_superclass is nicer.
> 
> I like your new comment, so have added it.

also I'm glad you suggested restructuring this code because now I can remove some parenthesis and read it.

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

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