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

[Ioi Lam]

On Wed, 18 Sep 2024 01:47:26 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   minor comment fix
>
> src/hotspot/share/cds/aotClassLinker.cpp line 121:
> 
>> 119:   assert(is_initialized(), "sanity");
>> 120: 
>> 121:   if (!CDSConfig::is_dumping_aot_linked_classes() || !SystemDictionaryShared::is_builtin(ik)) {
> 
> Shouldn't the CDSConfig check just be an assert - the caller is expected to check before trying to add candidates?

Fixed

> src/hotspot/share/cds/aotClassLinker.cpp line 145:
> 
>> 143:       return false;
>> 144:     }
>> 145:   }
> 
> Are we concerned with the possibility that we might be able to add some interfaces but not all, hence returning false, but with a subset of interfaces already added to the candidate list? I don't think it should be possible, but the code structure makes it look like it could be possible.

That's possible. Since we go through all the initial set of classes found in `ArchiveBuilder::current()->klasses()`, eventually all classes that are eligible for aot-linking will be added to the candidate list. The reason for the recursion is to

- Filter out classes whose super types are not eligible
- Sort the candidate list so that super types always come before sub types.

> src/hotspot/share/cds/aotClassLinker.cpp line 191:
> 
>> 189:     if (ik->class_loader() != class_loader) {
>> 190:       continue;
>> 191:     }
> 
> This seems very inefficient. We call `write_classes` 4 times, potentially with different loaders. Because the candidates are sorted the classes belonging to the same loader are likely to be grouped due to package names. So the app loader classes are likely to be right at end, and we have to traverse all the boot/platform classes first before we get to them. Conversely after we have encountered the last boot loader class (for example) we keep the scanning the entire list. If the set were ordered based on loader then name, we would be able to stop once we see the loader change to not being the desired one. And a binaery search would let you find the start of a section more quickly.

The classes are sorted by class hierarchy. It's a linear operation repeated 4 times, so it's not really something we need to optimize.

> src/hotspot/share/cds/aotClassLinker.cpp line 194:
> 
>> 192:     if ((ik->module() == ModuleEntryTable::javabase_moduleEntry()) != is_javabase) {
>> 193:       continue;
>> 194:     }
> 
> Why do we process system loader classes (i.e. application loader classes) if they need to be in java.base, as the application classes will never be in java.base. ???

The code is written for simplicity. If it becomes a performance problem we can change it.

> src/hotspot/share/cds/aotClassLinker.cpp line 198:
> 
>> 196:     if (ik->is_shared() && CDSConfig::is_dumping_dynamic_archive()) {
>> 197:       if (CDSConfig::is_using_aot_linked_classes()) {
>> 198:         // This class was recorded as a AOT-linked for the base archive,
> 
> Suggestion:
> 
>         // This class was recorded as AOT-linked for the base archive,

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115216
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115133
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115349
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115433
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115463