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

[Ioi Lam]

On Thu, 30 Jan 2025 17:34:04 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> There's an optimization in class loading that looks up class being loaded while loading its superclass, and if the class is found, it will check the superclass name against klass->_super and match class loaders. It's supposed to be a optimization to short circuit taking out the LOAD_SUPER (now called DETECT_CIRCULARITY) placeholder for the class being loaded.  So for example, loading class C, super S, we take out a DETECT_CIRCULARITY placeholder for C to detect if S then tries to load C as a super class by the same thread.
>> 
>> This optimization is almost never done because it requires just the right timing for a parallel thread to load the class right before this thread reaches the if statement (perhaps some long stall). The only case where this code is reached normally is for redefinition.  Loading a new class file version will find the class in the System Dictionary because we can only redefine already loaded classes.
>> 
>> It should be harmess to remove this if-statement in the case of redefinition also, but I think having a placeholder created for a known already-loaded class seems to break an understanding of how this works.  So I opted to just add to the comment and restructure the method a little bit.  The dictionary->find_class() call doesn't need to be inside the SystemDictionary_lock.
>> 
>> Tested with tier1-4 and internal parallel class loading tests.
>
> 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.

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

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