RFR: 8329706: Implement -XX:+AOTClassLinking [v3]

[Ioi Lam]

On Tue, 10 Sep 2024 10:08:35 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   @dholmes-ora comments: logging indents
>
> src/hotspot/share/cds/aotClassLinker.hpp line 71:
> 
>> 69: //
>> 70: class AOTClassLinker :  AllStatic {
>> 71:   using ClassesTable = ResourceHashtable<InstanceKlass*, bool, 15889, AnyObj::C_HEAP, mtClassShared>;
> 
> Can we have a symbolic name for this (prime) magic number here and in other places in this patch? I realise there is existing code which uses the raw number bit it is also consumed symbolically (e.g. in archiveBuilder.hpp, metaspaceClosure.hpp)

I added a `static const int TABLE_SIZE = 15889; // prime number`

> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 191:
> 
>> 189:   }
>> 190: 
>> 191:   ClassLoaderData* loader_data = ClassLoaderData::class_loader_data(loader());
> 
> Can we assert here that loader() != nullptr?

I added a few asserts in this function about the expected initiating and defining loaders.

> src/hotspot/share/cds/archiveBuilder.cpp line 433:
> 
>> 431:   }
>> 432: 
>> 433:   remember_embedded_pointer_in_enclosing_obj(ref);
> 
> I'm not clear why this was moved up. Was this just an omission (bug) in the earlier version or do we now need to remember a reference location that we could previously safely ignore?

If I remember correctly, this is a latent bug where we didn't remember the embedded pointer when the function returns early. This led to a crash that happened only when -XX:+AOTClassLinking is enabled.

> src/hotspot/share/cds/cdsConfig.cpp line 551:
> 
>> 549: }
>> 550: 
>> 551: void CDSConfig::set_has_aot_linked_classes(bool is_static_archive, bool has_aot_linked_classes) {
> 
> Why does this need to take `is_static_archive` as an argument?

This argument is unused and I removed it.

> src/hotspot/share/cds/dynamicArchive.cpp line 138:
> 
>> 136:     verify_estimate_size(_estimated_metaspaceobj_bytes, "MetaspaceObjs");
>> 137: 
>> 138:     sort_methods();
> 
> Could we have a comment to note that sorting and making shareable need to be done before calling `AOTClassLinker::write_to_archive();`

The reason I moved this code is I though it just looks odd -- we write a bunch of meta information about the classes, and then proceed on changing the contents of the classes.

The new order -- fix up the classes and then write the meta info -- should be quite natural, so I think a comment isn't needed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755536629
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755536786
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755536983
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755537234
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755537319