RFR: 8336919: Cleanup and rename tags in placeholders code

[Coleen Phillimore]

On Wed, 24 Jul 2024 21:38:55 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This change does some renaming of tags, fields and function name (resolve_super_or_fail => resolve_with_circularity_detection) in the Placeholders and SystemDictionary (class loading) code.  resolve_super_or_fail implied that it's always the super class, which it is not, especially with the upcoming changes to valhalla.  Renaming the function to resolve/load and check for circularity error is more accurate. These are changes made by @fparain brought over from the valhalla branch.
>> Tested with tier1-4.
>
> src/hotspot/share/classfile/classFileParser.cpp line 839:
> 
>> 837: 
>> 838:         // Call resolve_super so class circularity is checked
>> 839:         interf = SystemDictionary::resolve_with_circularity_detection(
> 
> Comment needs updating

Fixed.

> src/hotspot/share/classfile/classFileParser.cpp line 841:
> 
>> 839:         interf = SystemDictionary::resolve_with_circularity_detection(
>> 840:                                                   _class_name,
>> 841:                                                   unresolved_klass,
> 
> Parameter alignment is now incorrect

If I moved these out to the level of the (, they'd be too far to the right.  They are aligned and indented consistently with other code with long function names.

> src/hotspot/share/classfile/classFileParser.cpp line 5943:
> 
>> 5941:                                                                super_class_name,
>> 5942:                                                                loader,
>> 5943:                                                                _protection_domain,
> 
> Parameter alignment is now incorrect

Same here, they'd be too far to the right.

> src/hotspot/share/classfile/placeholders.cpp line 202:
> 
>> 200: // on store ordering here.
>> 201: static PlaceholderEntry* add_entry(Symbol* class_name, ClassLoaderData* loader_data,
>> 202:                             Symbol* next_klass_name){
> 
> Indent is wrong

Fixed.

> src/hotspot/share/classfile/placeholders.hpp line 90:
> 
>> 88:   SeenThread*       _loadInstanceThreadQ; // loadInstance thread
>> 89:                                     // This can't be multiple threads since class loading waits for
>> 90:                                     // this token to be removed.
> 
> Comments should line up

Fixed.

> src/hotspot/share/classfile/placeholders.hpp line 102:
> 
>> 100:   bool remove_seen_thread(JavaThread* thread, PlaceholderTable::classloadAction action);
>> 101: 
>> 102:   SeenThread*        circularityThreadQ()        const { return _circularityThreadQ; }
> 
> There's no alignment here so the (existing) extra space before `const` is not needed.

These are sort of lined up with loadInstanceThreadQ, so I fixed the alignment there.

Someday we should rename these queue names to follow Hotspot naming conventions, but I don't want to do it yet.

> src/hotspot/share/classfile/placeholders.hpp line 115:
> 
>> 113:      _circularityThreadQ(nullptr), _loadInstanceThreadQ(nullptr), _defineThreadQ(nullptr) { }
>> 114: 
>> 115:   Symbol*            next_klass_name()     const { return _next_klass_name; }
> 
> There's no alignment here so the (existing) extra space before `const` is not needed.

Pre-existing, it aligns with definer below.

> src/hotspot/share/classfile/systemDictionary.cpp line 412:
> 
>> 410:                                                        Handle protection_domain,
>> 411:                                                        bool is_superclass,
>> 412:                                                        TRAPS) {
> 
> Parameter alignment needs fixing

Fixed.

> src/hotspot/share/classfile/systemDictionary.cpp line 508:
> 
>> 506:   assert (!CDSConfig::is_dumping_static_archive(), "should not be parallel loading static archive");
>> 507:   Klass* superk = SystemDictionary::resolve_with_circularity_detection(name,
>> 508:                                                           superclassname,
> 
> Parameter alignment needs fixing

Also fixed.

> src/hotspot/share/classfile/systemDictionary.cpp line 511:
> 
>> 509:                                                           class_loader,
>> 510:                                                           protection_domain,
>> 511:                                                           false,
> 
> Please explain this change!

If you're doing a parallel super load, it could be the super class but not necessarily, it could also be an interface class.  I thought it was an optimization, because if you add the circularity placeholder, and look up the existing super class, it should immediately find it an return.  Now I'm wondering if it's more than an optimization based on the comment in resolve_with_circularity_check.  I'll check this.

I added the assert above it for the place in static dumping that used this flag as either klass->super or interface, ie where it differentiated based on this flag.

> src/hotspot/share/classfile/systemDictionary.cpp line 580:
> 
>> 578:   // name must be in the form of "java/lang/Object" -- cannot be "Ljava/lang/Object;"
>> 579:   assert(name != nullptr && !Signature::is_array(name) &&
>> 580:          !Signature::has_envelope(name), "invalid class name: %s", name == nullptr ? "nullptr" : name->as_C_string());
> 
> `name->as_C_String` requires a `ResourceMark` to be active - we probably should not assume/expect the caller has one.

Good catch.

> src/hotspot/share/classfile/systemDictionary.hpp line 108:
> 
>> 106:   // parse_interfaces, resolve_instance_class_or_null, load_shared_class
>> 107:   // "class_name" is the class whose super class or interface is being resolved.
>> 108:   static InstanceKlass* resolve_with_circularity_detection(Symbol* class_name,
> 
> Preceding comment needs updating if this is not necessarily for super class/interface.

The comment will be updated in the valhalla repo to include loadable constant attribute.

    // "class_name" is the class whose super class, interface or loadable constant attribute
    // is being resolved.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691325672
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691326401
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691326796
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691328521
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691331533
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691332916
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691333655
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691335207
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691335401
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691344002
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691346333
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1691350362