RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]

[Dan Heidinga]

On Sat, 25 May 2024 06:48:26 GMT, Ioi Lam <iklam at 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