On Thu, 23 May 2024 03:35:19 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 two additional commits since the
> last revision:
>
> - Merge branch 'master' into 8293980-resolve-fields-at-dumptime
> - 8293980: Resolve CONSTANT_FieldRef at CDS dump time
After making a quick first pass over this, I have some comments about the
constant pool and cpcache code.
src/hotspot/share/oops/constantPool.cpp line 301:
> 299: objArrayOop rr = resolved_references();
> 300: if (rr != nullptr) {
> 301: ConstantPool* orig_pool =
> ArchiveBuilder::current()->get_source_addr(this);
Are the changes below necessary? I think the original was fine but I may be
missing the point of this change.
src/hotspot/share/oops/constantPool.cpp line 464:
> 462: if (cache() != nullptr) {
> 463: // cache() is null if this class is not yet linked.
> 464: remove_resolved_field_entries_if_non_deterministic();
These methods look like they can belong to the constant pool cache instead. Can
cpCache call the ClassLinker code instead so this can be part of
`cache()->remove_unshareable_info()`?
src/hotspot/share/oops/constantPool.cpp line 520:
> 518: int cp_index = rfi->constant_pool_index();
> 519: bool archived = false;
> 520: bool resolved = rfi->is_resolved(Bytecodes::_putfield) ||
Is one of these meant to be `is_resolved(Bytecodes::get_field)` ?
src/hotspot/share/oops/resolvedFieldEntry.hpp line 65:
> 63: _tos_state = other._tos_state;
> 64: _flags = other._flags;
> 65: _get_code = other._get_code;
The fields `_get_code` and `_put_code` are normally set atomically, does this
need to be the case when copying as well?
-------------
PR Review: https://git.openjdk.org/jdk/pull/19355#pullrequestreview-2074929387
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1612265561
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1612288001
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1612277435
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1612261360
