Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Mon, 3 Jun 2024 19:13:54 GMT, Ioi Lam wrote: >> This makes sense. I will try to prototype it in the Leyden repo and then >> update this PR. > > I tried skipping the `methodHandle` parameter to > `InterpreterRuntime::resolve_get_put` but it's more complicated than I > thought. > > 1. The `fieldDescriptor::has_initialized_final_update()` will return true IFF > the class has `putfield` bytecodes to a final field outside of `` > methods. See > [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/rewriter.cpp#L463) > 2. When `InterpreterRuntime::resolve_get_put` is called for a `putfield`, it > adds `putfield` to the `ResolvedFieldEntry` only if > `fieldDescriptor::has_initialized_final_update()` is false. See > [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/interpreterRuntime.cpp#L703) > 3. `InterpreterRuntime::resolve_get_put`calls > `LinkResolver::resolve_field()`, which always checks if the `methodHandle` is > `` or not, without consulting > `fieldDescriptor::has_initialized_final_update()`. See > [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/linkResolver.cpp#L1040) > > (2) is an optimization -- if a method sets final fields only inside > `` methods, we should fully resolve the `putfield` bytecodes. > Otherwise every such `putfield` will result in a VM call. > > (3) is for correctness -- make sure that only `` can modify final > fields. > > I am pretty sure (2) and (3) are equivalent. I.e., we should check against > the method in (3) only if `fieldDescriptor::has_initialized_final_update()` > is true. However, (3) is security related code, so I don't want to change it > inside an optimization PR like this one. Without fixing that, I cannot call > `InterpreterRuntime::resolve_get_put` with a null `methodHandle`, as it will > hit the assert. > > This goes back to my original point -- I'd rather do something stupid but > correct (call the existing APIs and live with the existing behavior), rather > than trying to analyze the resolution code and see if we can skip certain > checks. I get what you're saying... and the `fieldDescriptor::has_initialized_final_update()` is a nice optimization that we don't want to mess up. Getting the straightforward code in makes sense as we can optimize it later given it runs at CDS archive build which isn't performance critical. Down the line, we can add new entry points into the LinkResolver for use by CDS that refuse to resolve final fields and avoid this kind of issue but that's a future problem. - PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1629702253
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Fri, 31 May 2024 18:43:49 GMT, Ioi Lam wrote:
>> The current algorithm says:
>>
>> for each bytecode in each method:
>> switch(bytecode) {
>> case getfield:
>> case outfield:
>> InterpreterRuntime::resolve_get_put(bc, raw_index, mh, cp, false
>> /*initialize_holder*/, CHECK);
>> break;
>>
>> }
>>
>> What I'm proposing is:
>>
>> for each ResolvedFieldEntry
>>bool success = InterpreterRuntime::resolve_get_put(getfield, raw_index,
>> nullptr /* mh */, cp, false /*initialize_holder*/, CHECK);
>>if (success) {
>> // also resolve for put
>> InterpreterRuntime::resolve_get_put(putfield, raw_index, nullptr /* mh
>> */, cp, false /*initialize_holder*/, CHECK);
>>}
>>
>>
>> The `method` parameter is not critical as the "current" algorithm attempts
>> resolution with multiple methods - once for each method that references the
>> ResolvedFieldEntry. The resolution logic already has to handle dealing with
>> different rules for different types of methods (ie `` & ``)
>> for normal execution and "knows" not to resolve entries (like puts of field
>> fields) regardless of the method as they need to do additional runtime
>> checks on every access.
>>
>> The same will apply to invoke bytecodes later it feels safer to do only
>> what the bytecodes in some method have asked for but the runtime already has
>> to be robust against different kinds of gets/puts or invokes targeting the
>> same cp entries. By eagerly resolving we're not giving up any safety.
>>
>> If you really want to only resolve the exact cases (ie: gets not puts, etc)
>> that were resolved in the training run, then we need to write out as part of
>> the classlist more explicitly what needs to be resolved:
>> ie:
>>
>> @cp_resolved_gets 4, 7 8
>> @cp_resolved_puts 7 8 10
>
> This makes sense. I will try to prototype it in the Leyden repo and then
> update this PR.
I tried skipping the `methodHandle` parameter to
`InterpreterRuntime::resolve_get_put` but it's more complicated than I thought.
1. The `fieldDescriptor::has_initialized_final_update()` will return true IFF
the class has `putfield` bytecodes to a final field outside of ``
methods. See
[here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/rewriter.cpp#L463)
2. When `InterpreterRuntime::resolve_get_put` is called for a `putfield`, it
adds `putfield` to the `ResolvedFieldEntry` only if
`fieldDescriptor::has_initialized_final_update()` is false. See
[here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/interpreterRuntime.cpp#L703)
3. `InterpreterRuntime::resolve_get_put`calls `LinkResolver::resolve_field()`,
which always checks if the `methodHandle` is `` or not, without
consulting `fieldDescriptor::has_initialized_final_update()`. See
[here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/linkResolver.cpp#L1040)
(2) is an optimization -- if a method sets final fields only inside ``
methods, we should fully resolve the `putfield` bytecodes. Otherwise every such
`putfield` will result in a VM call.
(3) is for correctness -- make sure that only `` can modify final fields.
I am pretty sure (2) and (3) are equivalent. I.e., we should check against the
method in (3) only if `fieldDescriptor::has_initialized_final_update()` is
true. However, (3) is security related code, so I don't want to change it
inside an optimization PR like this one. Without fixing that, I cannot call
`InterpreterRuntime::resolve_get_put` with a null `methodHandle`, as it will
hit the assert.
This goes back to my original point -- I'd rather do something stupid but
correct (call the existing APIs and live with the existing behavior), rather
than trying to analyze the resolution code and see if we can skip certain
checks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1624928922
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Fri, 31 May 2024 12:21:09 GMT, Dan Heidinga wrote:
>> If you look at the version in the Leyden repo, there are many different
>> types of references that are handled in
>> `ClassPrelinker::maybe_resolve_fmi_ref`
>>
>> https://github.com/openjdk/leyden/blob/4faa72029abb86b55cb33b00acf9f3a18ade4b77/src/hotspot/share/cds/classPrelinker.cpp#L307
>>
>> My goal is to defer all the safety checks to
>> `InterpreterRuntime::resolve_xxx` so that we don't need to think about what
>> is safe to pre-resolve, and what is not. Some of the checks are very complex
>> (see linkResolver.cpp as well) and may change as the language evolve.
>
> The current algorithm says:
>
> for each bytecode in each method:
> switch(bytecode) {
> case getfield:
> case outfield:
> InterpreterRuntime::resolve_get_put(bc, raw_index, mh, cp, false
> /*initialize_holder*/, CHECK);
> break;
>
> }
>
> What I'm proposing is:
>
> for each ResolvedFieldEntry
>bool success = InterpreterRuntime::resolve_get_put(getfield, raw_index,
> nullptr /* mh */, cp, false /*initialize_holder*/, CHECK);
>if (success) {
> // also resolve for put
> InterpreterRuntime::resolve_get_put(putfield, raw_index, nullptr /* mh
> */, cp, false /*initialize_holder*/, CHECK);
>}
>
>
> The `method` parameter is not critical as the "current" algorithm attempts
> resolution with multiple methods - once for each method that references the
> ResolvedFieldEntry. The resolution logic already has to handle dealing with
> different rules for different types of methods (ie `` & ``) for
> normal execution and "knows" not to resolve entries (like puts of field
> fields) regardless of the method as they need to do additional runtime checks
> on every access.
>
> The same will apply to invoke bytecodes later it feels safer to do only
> what the bytecodes in some method have asked for but the runtime already has
> to be robust against different kinds of gets/puts or invokes targeting the
> same cp entries. By eagerly resolving we're not giving up any safety.
>
> If you really want to only resolve the exact cases (ie: gets not puts, etc)
> that were resolved in the training run, then we need to write out as part of
> the classlist more explicitly what needs to be resolved:
> ie:
>
> @cp_resolved_gets 4, 7 8
> @cp_resolved_puts 7 8 10
This makes sense. I will try to prototype it in the Leyden repo and then update
this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1622828712
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Fri, 31 May 2024 00:22:37 GMT, Ioi Lam wrote:
>>> We could walk `_resolved_field_entries` to find the `ResolvedFieldEntry`
>>> whose `_cpool_index` is `123`. However, before the `ResolvedFieldEntry` is
>>> resolved, we don't know which bytecode is used to resolve it, so we don't
>>> know whether it's for a static field or non-static field. Since we want to
>>> filter out the static fields in the PR, we need to:
>>>
>>> * walk the bytecodes to find only getfield/putfield bytecodes
>>> * these bytecodes will give us an index to the `_resolved_field_entries`
>>> array
>>> * from there, we discover the original CP index
>>> * then we see if this index is set to true in `preresolve_list`
>>
>> Something's been bothering me about this explanation and I think I've put my
>> finger on it. As you show, the same CP entry can be referenced by both
>> `getstatic` & `getfield` bytecodes though only one will successfully
>> resolve. Walking the bytecodes doesn't actually tell us anything - the
>> resolution status should be different for instance vs static fields which
>> means we're should always be safe to attempt the resolution of fields as
>> instance fields provided we ignore errors.
>>
>>> So we must call `InterpreterRuntime::resolve_get_put()` which performs all
>>> the checks for access rights, static-vs-non-static, etc. This call requires
>>> a Method parameter, so we must walk all the Methods to find an appropriate
>>> one.
>>
>> The Method parameter is necessary for puts to final fields - either
>> `` for static finals or an `` method for instance finals. In
>> either case, the we don't actually resolve the field for puts so it doesn't
>> matter if we pass the "correct" method or not during pre resolution as it
>> will never successfully complete. I think we'd be OK to send any method we
>> want to that call when doing preresolution provided we ignore the errors
>
> If you look at the version in the Leyden repo, there are many different types
> of references that are handled in `ClassPrelinker::maybe_resolve_fmi_ref`
>
> https://github.com/openjdk/leyden/blob/4faa72029abb86b55cb33b00acf9f3a18ade4b77/src/hotspot/share/cds/classPrelinker.cpp#L307
>
> My goal is to defer all the safety checks to
> `InterpreterRuntime::resolve_xxx` so that we don't need to think about what
> is safe to pre-resolve, and what is not. Some of the checks are very complex
> (see linkResolver.cpp as well) and may change as the language evolve.
The current algorithm says:
for each bytecode in each method:
switch(bytecode) {
case getfield:
case outfield:
InterpreterRuntime::resolve_get_put(bc, raw_index, mh, cp, false
/*initialize_holder*/, CHECK);
break;
}
What I'm proposing is:
for each ResolvedFieldEntry
bool success = InterpreterRuntime::resolve_get_put(getfield, raw_index,
nullptr /* mh */, cp, false /*initialize_holder*/, CHECK);
if (success) {
// also resolve for put
InterpreterRuntime::resolve_get_put(putfield, raw_index, nullptr /* mh */,
cp, false /*initialize_holder*/, CHECK);
}
The method is not critical as the current algorithm attempts resolution with
multiple methods. The resolution logic already has to handle this for normal
execution and "knows" not to resolve entries (like puts of field fields)
regardless of the method as they need to do additional runtime checks.
The same will apply to invoke bytecodes later it feels safer to do only
what the bytecodes in some method have asked for but the runtime already has to
be robust against different kinds of gets/puts or invokes targeting the same cp
entries.
If you really want to only resolve the exact cases (ie: gets not puts, etc)
that were resolved in the training run, then we need to write out as part of
the classicist more explicitly what needs to be resolved:
ie:
@cp_resolved_gets 4, 7 8
@cp_resolved_puts 7 8 10
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1622343820
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Thu, 30 May 2024 04:15:24 GMT, Dan Heidinga wrote:
>> `preresolve_list` has the original CP indices (E.g., `putfield #123` as
>> stored in the classfile), but in HotSpot, after bytecode rewriting, the u2
>> following the bytecode is changed to an index into the
>> `cpcache()->_resolved_field_entries` array, so it becomes something like
>> `putfield #45`. So we need to know how to convert the `123` index to the
>> `45` index.
>>
>> We could walk `_resolved_field_entries` to find the `ResolvedFieldEntry`
>> whose `_cpool_index` is `123`. However, before the `ResolvedFieldEntry` is
>> resolved, we don't know which bytecode is used to resolve it, so we don't
>> know whether it's for a static field or non-static field. Since we want to
>> filter out the static fields in the PR, we need to:
>>
>> - walk the bytecodes to find only getfield/putfield bytecodes
>> - these bytecodes will give us an index to the `_resolved_field_entries`
>> array
>> - from there, we discover the original CP index
>> - then we see if this index is set to true in `preresolve_list`
>>
>> There's also a compatibility issue -- it's possible to have static and
>> non-static field access using the same CP index:
>>
>>
>> class Hack {
>> static int myField;
>> int foo(boolean flag) {
>> try {
>> if (flag) {
>> // throw IncompatibleClassChangeError
>> return /* pseudo code*/ getfield this.myField;
>> } else {
>> // OK
>> return /* pseudo code*/ getstatic Hack.myField;
>> }
>> } catch (Throwable) {
>> return 5678;
>> }
>> }
>>
>>
>> So we must call `InterpreterRuntime::resolve_get_put()` which performs all
>> the checks for access rights, static-vs-non-static, etc. This call requires
>> a Method parameter, so we must walk all the Methods to find an appropriate
>> one.
>>
>> Perhaps it's possible to refactor the `InterpreterRuntime` code to avoid
>> passing in a Method, but I am hesitant to do that with code that deals with
>> access right checks.
>
>> We could walk `_resolved_field_entries` to find the `ResolvedFieldEntry`
>> whose `_cpool_index` is `123`. However, before the `ResolvedFieldEntry` is
>> resolved, we don't know which bytecode is used to resolve it, so we don't
>> know whether it's for a static field or non-static field. Since we want to
>> filter out the static fields in the PR, we need to:
>>
>> * walk the bytecodes to find only getfield/putfield bytecodes
>> * these bytecodes will give us an index to the `_resolved_field_entries`
>> array
>> * from there, we discover the original CP index
>> * then we see if this index is set to true in `preresolve_list`
>
> Something's been bothering me about this explanation and I think I've put my
> finger on it. As you show, the same CP entry can be referenced by both
> `getstatic` & `getfield` bytecodes though only one will successfully resolve.
> Walking the bytecodes doesn't actually tell us anything - the resolution
> status should be different for instance vs static fields which means we're
> should always be safe to attempt the resolution of fields as instance fields
> provided we ignore errors.
>
>> So we must call `InterpreterRuntime::resolve_get_put()` which performs all
>> the checks for access rights, static-vs-non-static, etc. This call requires
>> a Method parameter, so we must walk all the Methods to find an appropriate
>> one.
>
> The Method parameter is necessary for puts to final fields - either
> `` for static finals or an `` method for instance finals. In
> either case, the we don't actually resolve the field for puts so it doesn't
> matter if we pass the "correct" method or not during pre resolution as it
> will never successfully complete. I think we'd be OK to send any method we
> want to that call when doing preresolution provided we ignore the errors
If you look at the version in the Leyden repo, there are many different types
of references that are handled in `ClassPrelinker::maybe_resolve_fmi_ref`
https://github.com/openjdk/leyden/blob/4faa72029abb86b55cb33b00acf9f3a18ade4b77/src/hotspot/share/cds/classPrelinker.cpp#L307
My goal is to defer all the safety checks to `InterpreterRuntime::resolve_xxx`
so that we don't need to think about what is safe to pre-resolve, and what is
not. Some of the checks are very complex (see linkResolver.cpp as well) and may
change as the language evolve.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1621541729
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Wed, 29 May 2024 16:03:35 GMT, Ioi Lam wrote: > We could walk `_resolved_field_entries` to find the `ResolvedFieldEntry` > whose `_cpool_index` is `123`. However, before the `ResolvedFieldEntry` is > resolved, we don't know which bytecode is used to resolve it, so we don't > know whether it's for a static field or non-static field. Since we want to > filter out the static fields in the PR, we need to: > > * walk the bytecodes to find only getfield/putfield bytecodes > * these bytecodes will give us an index to the `_resolved_field_entries` array > * from there, we discover the original CP index > * then we see if this index is set to true in `preresolve_list` Something's been bothering me about this explanation and I think I've put my finger on it. As you show, the same CP entry can be referenced by both `getstatic` & `getfield` bytecodes though only one will successfully resolve. Walking the bytecodes doesn't actually tell us anything - the resolution status should be different for instance vs static fields which means we're should always be safe to attempt the resolution of fields as instance fields provided we ignore errors. > So we must call `InterpreterRuntime::resolve_get_put()` which performs all > the checks for access rights, static-vs-non-static, etc. This call requires a > Method parameter, so we must walk all the Methods to find an appropriate one. The Method parameter is necessary for puts to final fields - either `` for static finals or an `` method for instance finals. In either case, the we don't actually resolve the field for puts so it doesn't matter if we pass the "correct" method or not during pre resolution as it will never successfully complete. I think we'd be OK to send any method we want to that call when doing preresolution provided we ignore the errors - PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1619844681
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Wed, 29 May 2024 12:53:57 GMT, Dan Heidinga wrote:
>> 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
>
> src/hotspot/share/cds/classListParser.cpp line 848:
>
>> 846: if (preresolve_fmi) {
>> 847: ClassPrelinker::preresolve_field_and_method_cp_entries(THREAD, ik,
>> _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?
`preresolve_list` has the original CP indices (E.g., `putfield #123` as stored
in the classfile), but in HotSpot, after bytecode rewriting, the u2 following
the bytecode is changed to an index into the
`cpcache()->_resolved_field_entries` array, so it becomes something like
`putfield #45`. So we need to know how to convert the `123` index to the `45`
index.
We could walk `_resolved_field_entries` to find the `ResolvedFieldEntry` whose
`_cpool_index` is `123`. However, before the `ResolvedFieldEntry` is resolved,
we don't know which bytecode is used to resolve it, so we don't know whether
it's for a static field or non-static field. Since we want to filter out the
static fields in the PR, we need to:
- walk the bytecodes to find only getfield/putfield bytecodes
- these bytecodes will give us an index to the `_resolved_field_entries` array
- from there, we discover the original CP index
- then we see if this index is set to true in `preresolve_list`
There's also a compatibility issue -- it's possible to have static and
non-static field access using the same CP index:
class Hack {
static int myField;
int foo(boolean flag) {
try {
if (flag) {
// throw IncompatibleClassChangeError
return /* pseudo code*/ getfield this.myField;
} else {
// OK
return /* pseudo code*/ getstatic Hack.myField;
}
} catch (Throwable) {
return 5678;
}
}
So we must call `InterpreterRuntime::resolve_get_put()` which performs all the
checks for access rights, static-vs-non-static, etc. This call requires a
Method parameter, so we must walk all the Methods to find an appropriate one.
Perhaps it's possible to refactor the `InterpreterRuntime` code to avoid
passing in a Method, but I am hesitant to do that with code that deals with
access right checks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1619144592
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Sat, 25 May 2024 06:48:26 GMT, Ioi Lam 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,
> _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
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
On Sat, 25 May 2024 06:48:26 GMT, Ioi Lam 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 Changes look good, thanks! - Marked as reviewed by matsaave (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19355#pullrequestreview-2083038736
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]
> ### 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 unnecessarily generate code for paths that are never taken by > the app... 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19355/files - new: https://git.openjdk.org/jdk/pull/19355/files/3900c568..89184c33 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19355=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19355=01-02 Stats: 13691 lines in 428 files changed: 7998 ins; 3129 del; 2564 mod Patch: https://git.openjdk.org/jdk/pull/19355.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19355/head:pull/19355 PR: https://git.openjdk.org/jdk/pull/19355
