On Fri, 13 Sep 2024 16:09:25 GMT, Ashutosh Mehra <asme...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @ashu-mehra comments
>
> src/hotspot/share/cds/aotClassLinker.cpp line 227:
>
>> 225: }
>> 226:
>> 227: int AOTClassLinker::num_initiated_classes(oop loader1, oop loader2) {
>
> The two loader arguments here are quite confusing marking it hard to
> understand the code. Can it be refactored as this:
>
>
> int AOTClassLinker::num_platform_initiated_classes() {
> // AOTLinkedClassBulkLoader will initiate loading of all public boot
> classes in the platform loader.
> return num_initiated_classes(nullptr);
> }
>
> int AOTClassLinker::num_app_initiated_classes() {
> // AOTLinkedClassBulkLoader will initiate loading of all public
> boot/platform classes in the app loader.
> return num_platform_initiated_classes +
> num_initiated_classes(SystemDictionary::java_platform_loader());
> }
>
> int AOTClassLinker::num_initiated_classes(oop loader) {
> int n = 0;
> for (int i = 0; i < _sorted_candidates->length(); i++) {
> InstanceKlass* ik = _sorted_candidates->at(i);
> if (ik->is_public() && !ik->is_hidden() &&
> (ik->class_loader() == loader) {
> n++;
> }
> }
>
> return n;
> }
I renamed the function to `AOTClassLinker::count_public_classes()` and made it
handle only a single loader each time. This function is not speed critical so I
think this way it's much easier to read.
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 199:
>
>> 197: InstanceKlass* ik = classes->at(i);
>> 198: assert(ik->is_loaded(), "must have already been loaded by a parent
>> loader");
>> 199: assert(ik->class_loader() != initiating_loader(), "must be a parent
>> loader");
>
> Can we also add an assert that ik->class_loader() must be either boot or
> platform loader.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1762007825
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1762007881
