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

[David Holmes]

On Tue, 28 Jan 2025 18:22:28 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:
> 
>   Pass false to check_is_superclass on further thought.

BTW the issue title says "pass pass"

src/hotspot/share/classfile/systemDictionary.cpp line 450:

> 448:     // Must check ClassCircularity before resolving next_name (superclass or interface).
> 449:     PlaceholderEntry* probe = PlaceholderTable::get_entry(class_name, loader_data);
> 450:     if (probe && probe->check_seen_thread(THREAD, PlaceholderTable::DETECT_CIRCULARITY)) {

Nit pre-existing use of implicit boolean - suggestion `probe != nullptr && ...`

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?

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

PR Comment: https://git.openjdk.org/jdk/pull/23322#issuecomment-2623508176
PR Review Comment: https://git.openjdk.org/jdk/pull/23322#discussion_r1934985157
PR Review Comment: https://git.openjdk.org/jdk/pull/23322#discussion_r1934997819