On Sat, 25 May 2024 06:48:26 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> ### Overview
>>
>> This PR archives `CONSTANT_FieldRef` entries in the _resolved_ state when
>> it's safe to do so.
>>
>> I.e., when a `CONSTANT_FieldRef` constant pool entry in class `A` refers to
>> a *non-static* field `B.F`,
>> - `B` is the same class as `A`; or
>> - `B` is a supertype of `A`; or
>> - `B` is one of the
>> [vmClasses](https://github.com/openjdk/jdk/blob/3d4185a9ce482cc655a4c67f39cb2682b02ae4fe/src/hotspot/share/classfile/vmClasses.hpp),
>> and `A` is loaded by the boot class loader.
>>
>> Under these conditions, it's guaranteed that whenever `A` tries to use this
>> entry at runtime, `B` is guaranteed to have already been resolved in A's
>> system dictionary, to the same value as resolved during dump time.
>>
>> Therefore, we can safely archive the `ResolvedFieldEntry` in class `A` that
>> refers to `B.F`.
>>
>> (Note that we do not archive the `CONSTANT_FieldRef` entries for static
>> fields, as the resolution of such entries can lead to class initialization
>> at runtime. We plan to handle them in a future RFE.)
>>
>> ### Static CDS Archive
>>
>> This feature is implemented in three steps for static CDS archive dump:
>>
>> 1. At the end of the training run, `ClassListWriter` iterates over all
>> loaded classes and writes the indices of their resolved `Class` and
>> `FieldRef` constant pool entries into the classlist file, with the `@cp`
>> prefix. E.g., the following means that the constant pool entries at indices
>> 2, 19 and 106 were resolved during the training run:
>>
>> @cp java/util/Objects 2 19 106
>>
>> 2. When creating the static CDS archive from the classlist file,
>> `ClassListParser` processes the `@cp` entries and resolves all the indicated
>> entries.
>>
>> 3. Inside the `ArchiveBuilder::make_klasses_shareable()` function, we
>> iterate over all entries in all archived `ConstantPools`. When we see a
>> _resolved_ entry that does not satisfy the safety requirements as stated in
>> _Overview_, we revert it back to the unresolved state.
>>
>> ### Dynamic CDS Archive
>>
>> When dumping the dynamic CDS archive, `ClassListWriter` and
>> `ClassListParser` are not used, so steps 1 and 2 are skipped. We only
>> perform step 3 when the archive is being written.
>>
>> ### Limitations
>>
>> - For safety, we limit this optimization to only classes loaded by the boot,
>> platform, and app class loaders. This may be relaxed in the future.
>> - We archive only the constant pool entries that are actually resolved
>> during the training run. We don't speculatively resolve other entries, as
>> doing so may cause C2 to...
>
> Ioi Lam has updated the pull request with a new target base due to a merge or
> a rebase. The incremental webrev excludes the unrelated changes brought in by
> the merge/rebase. The pull request contains five additional commits since the
> last revision:
>
> - Fixed typo in previous commit
> - Merge branch 'master' into 8293980-resolve-fields-at-dumptime
> - @matias9927 comments - moved
> remove_resolved_field_entries_if_non_deterministic() to cpCache
> - Merge branch 'master' into 8293980-resolve-fields-at-dumptime
> - 8293980: Resolve CONSTANT_FieldRef at CDS dump time
make/GenerateLinkOptData.gmk line 68:
> 66: # - The classlist can be influenced by locale. Always set it to en/US.
> 67: # - Run with -Xint, as the compiler can speculatively resolve constant
> pool entries.
> 68: # - ForkJoinPool parallelism can cause constant pool resolution to be
> non-dererministic.
Minor typo
Suggestion:
# - ForkJoinPool parallelism can cause constant pool resolution to be
non-deterministic.
src/hotspot/share/cds/classListParser.cpp line 848:
> 846: if (preresolve_fmi) {
> 847: ClassPrelinker::preresolve_field_and_method_cp_entries(THREAD, ik,
> &preresolve_list);
> 848: }
Can you clarify the approach here?
As I read the code, `ClassPrelinker::preresolve_class_cp_entries` will walk the
whole constant pool looking for unresolved class entries that match and then
resolve them. `ClassPrelinker::preresolve_field_and_method_cp_entries` walks
all methods bytecode by bytecode to resolve them.
Doesn't the `preresolve_list` already tell us which CP entries need to be
resolved and the cp tag tell us the type of resolution to do? Can we not do
this in a single pass over the cp rather than walking method bytecodes?
Is the reason for this approach to avoid always resolving FieldMethodRefs for
both get and put and only do them if there's a corresponding bytecode?
src/hotspot/share/oops/instanceKlass.cpp line 2560:
> 2558: // The ConstantPool is cleaned in a separate pass in
> ArchiveBuilder::make_klasses_shareable(),
> 2559: // so no need to do it here.
> 2560: //constants()->remove_unshareable_info();
Should this be deleted rather than commented out?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1617547809
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1618836313
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1617818205
