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

[Coleen Phillimore]

On Fri, 31 Jan 2025 05:33:07 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix implicit nullptr check.
>
> 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.

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

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