On Wed, 18 Sep 2024 01:47:26 GMT, David Holmes <dhol...@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
