On Tue, 10 Sep 2024 10:08:35 GMT, Andrew Dinn <ad...@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
