Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v5]
> ### 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 incrementally with two additional commits since the last revision: - Added test case for safety with putfield against final fields (related to JDK-8157181) - Moved the test ResolvedConstants.java to resolvedConstants, as we will have more tests cases in this area - Changes: - all: https://git.openjdk.org/jdk/pull/19355/files - new: https://git.openjdk.org/jdk/pull/19355/files/17a1ce62..58e08e18 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19355=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19355=03-04 Stats: 161 lines in 3 files changed: 160 ins; 1 del; 0 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
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 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 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 [v4]
> ### 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 incrementally with one additional commit since the last revision: @DanHeidinga comments - Changes: - all: https://git.openjdk.org/jdk/pull/19355/files - new: https://git.openjdk.org/jdk/pull/19355/files/89184c33..17a1ce62 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19355=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19355=02-03 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 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
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v2]
On Thu, 23 May 2024 20:50:47 GMT, Matias Saavedra Silva
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 two additional commits
>> since the last revision:
>>
>> - Merge branch 'master' into 8293980-resolve-fields-at-dumptime
>> - 8293980: Resolve CONSTANT_FieldRef at CDS dump time
>
> 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()`?
I moved remove_resolved_field_entries_if_non_deterministic() to cpCache as you
suggested. I removed the functions for indy and method, as dumptime resolution
for those types of entries is not yet implemented.
> 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)` ?
Fixed.
> 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?
This is done inside while the ResolvedFieldEntries are being prepared during
class rewriting. All access is single threaded so there's no need for atomic
operations.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1614389410
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1614389465
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1614390838
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
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v2]
On Thu, 23 May 2024 20:28:49 GMT, Matias Saavedra Silva
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 two additional commits
>> since the last revision:
>>
>> - Merge branch 'master' into 8293980-resolve-fields-at-dumptime
>> - 8293980: Resolve CONSTANT_FieldRef at CDS dump time
>
> 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.
It's just for consistency. "source" is the terminology used in the comments in
archiveBuilder.cpp.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1612386810
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time
On Wed, 22 May 2024 21:48:44 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 unnecessarily generate code for paths that are never taken by > the app... I pressed the wrong button and sent out the RFR mail too soon I have finished updating the PR description text. It's ready for review now. - PR Comment: https://git.openjdk.org/jdk/pull/19355#issuecomment-2126167774
Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v2]
> ### 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 two additional commits since the last revision: - 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/6a3dc649..3900c568 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19355=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19355=00-01 Stats: 289 lines in 23 files changed: 123 ins; 139 del; 27 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
RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time
This PR tries store CONSTANT_FieldRef entries in the resolved state whenever it's safe to do so. I.e., when a constant pool entry in class `A` refers to a *non-static* field `B.F`, - `B` must be 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 run time, `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 this - Commit messages: - 8293980: Resolve CONSTANT_FieldRef at CDS dump time Changes: https://git.openjdk.org/jdk/pull/19355/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19355=00 Issue: https://bugs.openjdk.org/browse/JDK-8293980 Stats: 1134 lines in 30 files changed: 969 ins; 57 del; 108 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
Re: RFR: 8331942: On Linux aarch64, CDS archives should be using 64K alignment by default
On Wed, 8 May 2024 15:14:16 GMT, Thomas Stuefe wrote: > On Linux aarch64, a JVM may encounter three different page sizes: 4K, 64K, > and (when run on Mac M1 hardware) 16K. > > Since forgetting to specify `--enable-compatible-cds-alignment` is a common > error that is only noticed when running the produced JVM on hardware with > different page size, we propose to enable that option by default on Linux > aarch64. The cost is a moderate increase in CDS archive size (about 300K). > > I tested this patch on: > - x64 Linux > - x64 Linux, crossbuilding to aarch64 > - building natively on aarch64 Linux Looks good to me. I will close my JBS issue as a duplicate of this issue. Thanks - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19142#pullrequestreview-2048421270
Re: RFR: JDK-8300531: MSVC CFlags look suspicious [v9]
On Wed, 15 Feb 2023 21:21:47 GMT, Justin King wrote: >> Update MSVC CFlags to be more consistent with other compilers. Also disables >> RTTI in a simliar manner to GCC/Clang for the JVM. > > Justin King has updated the pull request incrementally with one additional > commit since the last revision: > > Revert usage of /GR- > > Signed-off-by: Justin King As a general comment, the bug title should be to the point. "Look suspicious" contains no information. It looks like you are changing the optimization level, so the bug title should reflect that. - Changes requested by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/12073
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions [v11]
On Thu, 19 Jan 2023 20:10:23 GMT, Matias Saavedra Silva wrote: >> This is an enhancement of the test case in >> [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests >> against an archive created by the "boot JDK", which is usually set as the >> previous official JDK release when building the JDK repo. >> >> If it's able to acquire previous valid JDK releases: >> - Download and install previous JDK versions (19 through N) >> where N == java.lang.Runtime.version().major() - 1 >> - Test the interaction of the current JDK versus each of the previous >> releases >> >> If it's not able to find the previous releases revert to the existing logic >> in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or >> test.previous.jdk properties). Verified with tier1-4 tests. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Corrected comments LGTM. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions [v9]
On Fri, 13 Jan 2023 17:05:10 GMT, Matias Saavedra Silva
wrote:
>> This is an enhancement of the test case in
>> [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests
>> against an archive created by the "boot JDK", which is usually set as the
>> previous official JDK release when building the JDK repo.
>>
>> If it's able to acquire previous valid JDK releases:
>> - Download and install previous JDK versions (19 through N)
>> where N == java.lang.Runtime.version().major() - 1
>> - Test the interaction of the current JDK versus each of the previous
>> releases
>>
>> If it's not able to find the previous releases revert to the existing logic
>> in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or
>> test.previous.jdk properties). Verified with tier1-4 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Added exceptions instead of default jdk
Changes requested by iklam (Reviewer).
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 104:
> 102: newJVM = TEST_JDK + FS + "bin" + FS + "java";
> 103:
> 104: // Example path:
> bundles/linux-x64/jdk-19_linux-x64_bin.tar.gz/jdk-19/bin/java
It's not cleat what this comment refers to. I think it can be deleted.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 172:
> 170:
> 171: // Fetch JDK artifact depending on platform
> 172: // If the artifact cannot be found, default to the test.boot.jdk
> property
This comment about test.boot.jdk is no longer valid. If the artifact cannot be
found, RuntimeException is thrown.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 199:
> 197: architecture = "x";
> 198: } else if (Platform.isAArch64()) {
> 199: architecture = "aarch";
It's better to use "x64" and "aarch64" here. The comments below should be
fixed, too.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 207:
> 205: // Ex: bundles/linux-x64/jdk-19_linux-x64_bin.tar.gz
> 206: if (Platform.isWindows()) {
> 207: jdkArtifactMap.put("file", "bundles/windows-x64/jdk-" +
> version + "_windows-x64_bin.zip");
Instead of hard-coding "x64", `architecture` should be used.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 225:
> 223: try {
> 224: path = ArtifactResolver.resolve("jdk", jdkArtifactMap, true)
> + "/jdk-" + version;
> 225: System.out.println("Boot JDK path: " + path);
`String path` should be declared inside this `try` block and its value should
be returned here. That way you don't need to read all the catch block below to
find out what value is actually returned.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 230:
> 228: if (cause == null) {
> 229: System.out.println("Cannot resolve artifact, "
> 230: + "please check if JIB jar is present in
> classpath.");
If we come to here, the function would end up returning null. We shold always
throw the RuntimeException when we come into this `catch` block.
Also, I am not sure if we should assume that `e.getCause() == null` has any
specific meaning. There could be multiple implementations of
`ArtifactResolver.resolve()` and we can't predict what the `e.getCause()` would
be.
-
PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v12]
On Tue, 10 Jan 2023 23:17:06 GMT, Justin King wrote:
>> This change instruments Metaspace for ASan. Metaspace allocates memory using
>> `mmap`/`munmap` which ASan is not aware of. Fortunately ASan supports
>> applications [manually poisoning/unpoisoning
>> memory](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning).
>> ASan is able to detect poisoned memory, similar to `use-after-free`, and
>> will raise an error similarly called `use-after-poison`. This provides and
>> extra layer of defense and confidence.
>>
>> The header `sanitizers/address.h` defines macros for poisoning/unpoisoning
>> memory regions. These macros can be used regardless of build mode. When ASan
>> is not available, they are implemented using a NOOP approach which still
>> compiles the arguments but does so such that they will be stripped out by
>> the compiler due to being unreachable. This helps with maintenance.
>>
>> This also has the added benefit of making
>> [LSan](https://bugs.openjdk.org/browse/JDK-8298445) more accurate and
>> deterministic, as LSan will not look for pointers to malloc memory in
>> poisoned memory regions.
>>
>> IMO the benefit of doing this greatly outweighs the cost.
>
> Justin King has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update sanitizers/address.h based on review
>
> Signed-off-by: Justin King
src/hotspot/share/memory/metaspace/chunkManager.cpp line 308:
> 306: }
> 307:
> 308: if (enlarged) {
What's the reason of calling the ASAN functions outside of the lock?
Will we have a race condition if two threads call
ChunkManager::attempt_enlarge_chunk() at the same time and get to here out of
order? @tstuefe
-
PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions [v6]
On Thu, 12 Jan 2023 03:02:51 GMT, Matias Saavedra Silva
wrote:
>> This is an enhancement of the test case in
>> [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests
>> against an archive created by the "boot JDK", which is usually set as the
>> previous official JDK release when building the JDK repo.
>>
>> If it's able to acquire previous valid JDK releases:
>> - Download and install previous JDK versions (19 through N)
>> where N == java.lang.Runtime.version().major() - 1
>> - Test the interaction of the current JDK versus each of the previous
>> releases
>>
>> If it's not able to find the previous releases revert to the existing logic
>> in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or
>> test.previous.jdk properties). Verified with tier1-4 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Changed variable names and consolidated redundant code
Changes requested by iklam (Reviewer).
test/lib/jdk/test/lib/artifacts/ArtifactManager.java line 31:
> 29: public interface ArtifactManager {
> 30: public Path resolve(Artifact artifact) throws
> ArtifactResolverException;
> 31: Path resolve(String name, Map artifactDescription,
> boolean unpack) throws ArtifactResolverException;
For consistency, `public` should be added, similar to the existing interface
method.
Also, ArtifactManager is a public interface and some JDK developers may have
written custom classes of this interface. If we add a new interface method, we
may break such custom classes.
For compatibility, the new method should be declared with a default
implementation:
default public Path resolve(String name, Map
artifactDescription, boolean unpack) throws ArtifactResolverException {
throw new ArtifactResolverException("not implemented");
}
Note that it throws the exception instead of returning null. This will be
consistent with the existing interface method, which throws
ArtifactResolverException in case of any error.
Otherwise, the caller of the new method would need to handle both the null
return and the ArtifactResolverException, which would be too clumsy.
test/lib/jdk/test/lib/artifacts/DefaultArtifactManager.java line 48:
> 46: @Override
> 47: public Path resolve(String name, Map
> artifactDescription, boolean unpack) {
> 48: return null;
This should be removed as we already have a default implementation.
-
PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v11]
On Tue, 10 Jan 2023 17:27:33 GMT, Justin King wrote: >> This change instruments Metaspace for ASan. Metaspace allocates memory using >> `mmap`/`munmap` which ASan is not aware of. Fortunately ASan supports >> applications [manually poisoning/unpoisoning >> memory](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning). >> ASan is able to detect poisoned memory, similar to `use-after-free`, and >> will raise an error similarly called `use-after-poison`. This provides and >> extra layer of defense and confidence. >> >> The header `sanitizers/address.h` defines macros for poisoning/unpoisoning >> memory regions. These macros can be used regardless of build mode. When ASan >> is not available, they are implemented using a NOOP approach which still >> compiles the arguments but does so such that they will be stripped out by >> the compiler due to being unreachable. This helps with maintenance. >> >> This also has the added benefit of making >> [LSan](https://bugs.openjdk.org/browse/JDK-8298445) more accurate and >> deterministic, as LSan will not look for pointers to malloc memory in >> poisoned memory regions. >> >> IMO the benefit of doing this greatly outweighs the cost. > > Justin King has updated the pull request incrementally with one additional > commit since the last revision: > > Use macros from when available and update > justification > > Signed-off-by: Justin King src/hotspot/share/sanitizers/address.h line 45: > 43: #else > 44: #define NO_SANITIZE_ADDRESS > 45: #endif The `NO_SANITIZE_ADDRESS` macro doesn't seem to be used by this patch. src/hotspot/share/sanitizers/address.h line 53: > 51: // . When ASan is not available this macro is > a NOOP which preserves the > 52: // arguments, ensuring they still compile, but ensures they are stripped > due to being unreachable. > 53: // This helps ensure developers do not accidently break ASan builds. Maybe the "When ASan is available ... do not accidently break ASan builds." parts can be combined for the two macros to avoid duplication? - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Mon, 9 Jan 2023 15:48:48 GMT, Justin King wrote: >> I like this, but would compilers not complain about unused statements? > > I don't believe so, at least not based on current options. Other projects use > similar tricks, like ABSL_DCHECK from Abseil which short circuits expressions > when in non-debug builds causing the code to be unreachable and stripped. If > it does, we can always just remove the if statement part and have it evaluate > anyway. Most compilers should realize that the statements are unused and > should be pruned. The macros `ASAN_POISON_MEMORY_REGION` and `ASAN_UNPOISON_MEMORY_REGION` are already defined in [sanitizer/asan_interface.h](https://opensource.apple.com/source/clang/clang-700.1.81/src/projects/compiler-rt/include/sanitizer/asan_interface.h.auto.html), but we redefine them here. Since ASan is going to be an obscured feature for many HotSpot developers, I don't think we should make it more obscured. I think it's better to declare our own macros that call into the existing macros. Maybe something like `_ASAN_POISON_MEMORY_REGION`. The comments should be improved to say why we do this (to prevent people that don't use ASan from breaking ASan). - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions [v5]
On Fri, 6 Jan 2023 22:23:35 GMT, Matias Saavedra Silva wrote: >> This is an enhancement of the test case in >> [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests >> against an archive created by the "boot JDK", which is usually set as the >> previous official JDK release when building the JDK repo. >> >> If it's able to acquire previous valid JDK releases: >> - Download and install previous JDK versions (19 through N) >> where N == java.lang.Runtime.version().major() - 1 >> - Test the interaction of the current JDK versus each of the previous >> releases >> >> If it's not able to find the previous releases revert to the existing logic >> in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or >> test.previous.jdk properties). Verified with tier1-4 tests. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Test requires 64 bit test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java line 41: > 39: import jdk.test.lib.artifacts.Artifact; > 40: import jdk.test.lib.artifacts.ArtifactResolver; > 41: import jdk.test.lib.artifacts.ArtifactResolverException; Some of these new imports are not needed anymore. Also, there's other code that need to be removed, like the `props` below. - PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions [v2]
On Fri, 6 Jan 2023 13:47:22 GMT, Erik Joelsson wrote:
>> Matias Saavedra Silva 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 eight
>> additional commits since the last revision:
>>
>> - Fixed jib.sh
>> - Removed prints
>> - Ioi comments and jib.sh restored
>> - Merge branch 'master' into sharedArchiveTest_8287873
>> - Removing unused code
>> - Removed file added by mistake
>> - Defaults to old functionality on failure
>> - 8287873: Add test for using -XX:+AutoCreateSharedArchive with different
>> JDK versions
>
> test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
> line 62:
>
>> 60: // "make test
>> TEST=test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java",
>> 61: // the test.boot.jdk property is normally passed by make/RunTests.gmk
>> 62: private static String BOOT_JDK;
>
> Could we please call this something else? The concept of the BOOT_JDK is a
> rather specific thing in the JDK build process. This test is using the
> build's BOOT_JDK as a default for "a JDK of some older version than the
> current". Calling that "BOOT_JDK" in this test is confusing at least to me.
>
> Perhaps something like `OLD_JDK` would be more suitable?
Actually OLD_JDK would be confusing because we also have PREV_JDK. This is the
original code before this PR:
// If you're running this test manually, specify the location of a previous
version of
// the JDK using "jtreg -vmoption:-Dtest.previous.jdk=${JDK19_HOME} ..."
private static final String PREV_JDK = System.getProperty("test.previous.jdk",
null);
// If you're unning this test using something like
// "make test
TEST=test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java",
// the test.boot.jdk property is passed by make/RunTests.gmk
private static final String BOOT_JDK = System.getProperty("test.boot.jdk",
null);
So the problem with this PR is it overloads the meaning of BOOT_JDK. I think
it's better to do something like this:
static void setupJVMs(int fetchVersion) throws Throwable {
if (fetchVersion> 0) {
oldJVM = fetchJDK(fetchVersion) + FS + "bin" + FS + "java";
} else if (PREV_JDK != null) {
oldJVM = PREV_JDK + FS + "bin" + FS + "java";
} else if (BOOT_JDK != null) {
oldJVM = BOOT_JDK + FS + "bin" + FS + "java";
} else {
-
PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions
On Thu, 5 Jan 2023 19:25:07 GMT, Coleen Phillimore wrote:
>> This is an enhancement of the test case in
>> [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests
>> against an archive created by the "boot JDK", which is usually set as the
>> previous official JDK release when building the JDK repo.
>>
>> If it's able to acquire previous valid JDK releases:
>> - Download and install previous JDK versions (19 through N)
>> where N == java.lang.Runtime.version().major() - 1
>> - Test the interaction of the current JDK versus each of the previous
>> releases
>>
>> If it's not able to find the previous releases revert to the existing logic
>> in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or
>> test.previous.jdk properties). Verified with tier1-4 tests.
>
> test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
> line 80:
>
>> 78:
>> 79: // Earliest testable version is 19
>> 80: int n = java.lang.Runtime.version().major() - 1;
>
> Can this just do n-1 and not download every release? So if JDK 20 works with
> JDK 19, then JDK 21 works with JDK 20 and transitively would work with 19?
I think we should start the loop from n-1 and go downwards. After the first
iteration:
if (System.getProperty("test.autocreatesharedarchive.all.jdk.versions") ==
null) {
break;
}
This should be enough for day-to-day testing. For thoroughness, we can add a
task at tier 7 or 8 to specify
`-Dtest.autocreatesharedarchive.all.jdk.versions` and check against all
applicable JDK versions.
-
PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions
On Wed, 4 Jan 2023 20:07:48 GMT, Matias Saavedra Silva
wrote:
> This is an enhancement of the test case in
> [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests
> against an archive created by the "boot JDK", which is usually set as the
> previous official JDK release when building the JDK repo.
>
> If it's able to connect to an artifactory that hosts valid JDK releases:
> - Download and install previous JDK versions (19 through N) from the
> artifactory
> where N == java.lang.Runtime.version().major() - 1
> - test the interaction of the current JDK versus each of the previous
> releases.
>
> If it's not able to connect to such an artifactory, revert to the existing
> logic in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or
> test.previous.jdk properties). Verified with tier1-4 tests.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 209:
> 207: case "Windows":
> 208: jdkArtifactMap.put("version", version);
> 209: jdkArtifactMap.put("build_number", build);
The above two lines can be shared across all oses.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 256:
> 254: }
> 255: return osName;
> 256: }
Instead of writing your own code for detecting OS/architecture, etc, you should
use [test/lib/jdk/test/lib/Platform.java
](https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/Platform.java)
-
PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions
On Wed, 4 Jan 2023 20:07:48 GMT, Matias Saavedra Silva wrote: > This is an enhancement of the test case in > [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests > against an archive created by the "boot JDK", which is usually set as the > previous official JDK release when building the JDK repo. > > If it's able to connect to an artifactory that hosts valid JDK releases: > - Download and install previous JDK versions (19 through N) from the > artifactory > where N == java.lang.Runtime.version().major() - 1 > - test the interaction of the current JDK versus each of the previous > releases. > > If it's not able to connect to such an artifactory, revert to the existing > logic in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or > test.previous.jdk properties). Verified with tier1-4 tests. test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java line 101: > 99: oldJVM = (os == "MacOSX") ? > 100: BOOT_JDK + ".jdk" + FS + "Contents" + FS + "Home" + FS + > "bin" + FS + "java" : > 101: BOOT_JDK + FS + "bin" + FS + "java"; As mentioned in a comment in [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), we should change the RuntimeException in the next block to a SkippedException. - PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: 8287873: Add test for using -XX:+AutoCreateSharedArchive with different JDK versions
On Wed, 4 Jan 2023 20:07:48 GMT, Matias Saavedra Silva
wrote:
> This is an enhancement of the test case in
> [JDK-8296754](https://bugs.openjdk.org/browse/JDK-8296754), which tests
> against an archive created by the "boot JDK", which is usually set as the
> previous official JDK release when building the JDK repo.
>
> If it's able to connect to an artifactory that hosts valid JDK releases:
> - Download and install previous JDK versions (19 through N) from the
> artifactory
> where N == java.lang.Runtime.version().major() - 1
> - test the interaction of the current JDK versus each of the previous
> releases.
>
> If it's not able to connect to such an artifactory, revert to the existing
> logic in TestAutoCreateSharedArchiveUpgrade.java (use the test.boot.jdk or
> test.previous.jdk properties). Verified with tier1-4 tests.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
line 85:
> 83: setupJVMs(os);
> 84: doTest();
> 85: }
If fetchBootJDK() cannot connect to the artifactory, it will always returns the
same path regardless of the version. In that case, we shouldn't call doTest()
more than once.
test/lib/jdk/test/lib/artifacts/Artifact.java line 44:
> 42: int version() default 0;
> 43: int build_number() default 0;
> 44: String file() default "";
Are the changes in this file still needed? I think you were using Artifact and
annotations in an earlier version of TestAutoCreateSharedArchiveUpgrade.java,
but that would result in lots of duplications across {JDK versions} x {
platforms}. That's why you chose to go with a lower-level API (the new
ArtifactResolver.::resolve() method) so you can build the Map programmatically.
-
PR: https://git.openjdk.org/jdk/pull/11852
Re: RFR: 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 [v7]
On Tue, 22 Nov 2022 19:21:51 GMT, Matias Saavedra Silva wrote: >> The -XX:+AutoCreateSharedArchive flag was implemented in JDK 19, however, >> this flag doesn't work across JDK 19 and 20. >> >> Expected: JDK 20 should recreate the specified CDS archive >> Actual: JDK 20 cannot recognize the archive file and gives up >> >> The new field from GenericCDSFileMapHeader is now in FileMapHeader. Verified >> with tier 1-4 tests. > > Matias Saavedra Silva 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 eight additional > commits since the last revision: > > - Merge branch 'master' into autoCreateSharedArchive_8296754 > - Added newline to end of test > - Removed unused import > - Test now passes > - Changed whitespace and comment > - Adjusted whitespace > - Added test > - 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 Approved latest changes again. Matias had to do second force-push which reverted the effects of the first force-push. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/11148
Re: RFR: 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 [v5]
On Thu, 17 Nov 2022 21:46:45 GMT, Matias Saavedra Silva wrote: >> The -XX:+AutoCreateSharedArchive flag was implemented in JDK 19, however, >> this flag doesn't work across JDK 19 and 20. >> >> Expected: JDK 20 should recreate the specified CDS archive >> Actual: JDK 20 cannot recognize the archive file and gives up >> >> The new field from GenericCDSFileMapHeader is now in FileMapHeader. Verified >> with tier 1-4 tests. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Added newline to end of test LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/11148
Integrated: 8293293: Move archive heap loading code out of heapShared.cpp
On Fri, 2 Sep 2022 05:31:35 GMT, Ioi Lam wrote: > I moved all code related to loading the archive heap regions into a new file, > archiveHeapLoader.cpp. > > A diff of the new archiveHeapLoader.cpp and the old heapShared.cpp shows that > the moved code is identical, except for the change of `HeapShared::` to > `ArchiveHeapLoader::`. > > I also removed unnecessary entries in JVM_EXCLUDE_FILES that are already > covered by the `cds/` pattern. This pull request has now been integrated. Changeset: ac05bc86 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/ac05bc8605bcf343f0c230868af3056f03461e01 Stats: 1294 lines in 19 files changed: 683 ins; 563 del; 48 mod 8293293: Move archive heap loading code out of heapShared.cpp Reviewed-by: erikj, coleenp - PR: https://git.openjdk.org/jdk/pull/10138
Re: RFR: 8293293: Move archive heap loading code out of heapShared.cpp [v2]
On Fri, 2 Sep 2022 12:42:36 GMT, Erik Joelsson wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Cleaned up header files > > Build change looks good. Thanks for the review @erikj79 @coleenp - PR: https://git.openjdk.org/jdk/pull/10138
Re: RFR: 8293293: Move archive heap loading code out of heapShared.cpp [v2]
On Fri, 2 Sep 2022 17:16:42 GMT, Coleen Phillimore wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Cleaned up header files
>
> src/hotspot/share/cds/archiveHeapLoader.cpp line 58:
>
>> 56: #include "oops/oop.inline.hpp"
>> 57: #include "oops/typeArrayOop.inline.hpp"
>> 58: #include "prims/jvmtiExport.hpp"
>
> It looks like you should be able to remove a lot of these header files.
Thanks for the review. I took out the unnecessary includes in the four affected
files {heapShared,archiveHeapLoader}.{cpp,hpp}. As a result, I had to fix a few
other files to add missing header files that were included transitively.
-
PR: https://git.openjdk.org/jdk/pull/10138
Re: RFR: 8293293: Move archive heap loading code out of heapShared.cpp [v2]
> I moved all code related to loading the archive heap regions into a new file, > archiveHeapLoader.cpp. > > A diff of the new archiveHeapLoader.cpp and the old heapShared.cpp shows that > the moved code is identical, except for the change of `HeapShared::` to > `ArchiveHeapLoader::`. > > I also removed unnecessary entries in JVM_EXCLUDE_FILES that are already > covered by the `cds/` pattern. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: Cleaned up header files - Changes: - all: https://git.openjdk.org/jdk/pull/10138/files - new: https://git.openjdk.org/jdk/pull/10138/files/35c14e8d..7acf47bf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10138=01 - incr: https://webrevs.openjdk.org/?repo=jdk=10138=00-01 Stats: 60 lines in 8 files changed: 7 ins; 49 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/10138.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10138/head:pull/10138 PR: https://git.openjdk.org/jdk/pull/10138
RFR: 8293293: Move archive heap loading code out of heapShared.cpp
I moved all code related to loading the archive heap regions into a new file, archiveHeapLoader.cpp. A diff of the new archiveHeapLoader.cpp and the old heapShared.cpp shows that the moved code is identical, except for the change of `HeapShared::` to `ArchiveHeapLoader::`. I also removed unnecessary entries in JVM_EXCLUDE_FILES that are already covered by the `cds/` pattern. - Commit messages: - 8293293: Move archive heap loading code out of heapShared.cpp Changes: https://git.openjdk.org/jdk/pull/10138/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10138=00 Issue: https://bugs.openjdk.org/browse/JDK-8293293 Stats: 1318 lines in 19 files changed: 716 ins; 554 del; 48 mod Patch: https://git.openjdk.org/jdk/pull/10138.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10138/head:pull/10138 PR: https://git.openjdk.org/jdk/pull/10138
Re: RFR: 8292329: Enable CDS shared heap for zero builds [v2]
On Wed, 24 Aug 2022 06:04:52 GMT, Aleksey Shipilev wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @shipilev review comments > > Zero can default to G1 after #9994. Together with this patch, it should also > be able to use the shared heap from CDS, I think. It also tells me that it is > good to force G1 at CDS dump time, even without Zero, because the ergonomics > might select Serial on some machines. (Like, as I remember from the > `compilerDefinitions.cpp` code, might happen on Windows x86_32 that drops to > "client emulation" mode.). Thanks @shipilev @erikj79 @magicus for the review. - PR: https://git.openjdk.org/jdk/pull/9984
Integrated: 8292329: Enable CDS shared heap for zero builds
On Tue, 23 Aug 2022 15:56:35 GMT, Ioi Lam wrote: > ZERO uses UseSerialGC by default. When we dump the default CDS archive during > the build process, it fails to dump the shared heap (which requires G1GC). > > The fix is to force -XX:+UseG1GC when dumping the default CDS archive during > the build process. > > Speed up: > > (Before) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > (After) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.011986 +- 0.000205 seconds time elapsed ( +- 1.71% ) This pull request has now been integrated. Changeset: 76ee5495 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/76ee5495cd00f5546a5748051cc36965a8e936db Stats: 13 lines in 1 file changed: 11 ins; 0 del; 2 mod 8292329: Enable CDS shared heap for zero builds Reviewed-by: shade, erikj, ihse - PR: https://git.openjdk.org/jdk/pull/9984
Re: RFR: 8292329: Enable CDS shared heap for zero builds [v3]
> ZERO uses UseSerialGC by default. When we dump the default CDS archive during > the build process, it fails to dump the shared heap (which requires G1GC). > > The fix is to force -XX:+UseG1GC when dumping the default CDS archive during > the build process. > > Speed up: > > (Before) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > (After) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.011986 +- 0.000205 seconds time elapsed ( +- 1.71% ) Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @magicus comments - Changes: - all: https://git.openjdk.org/jdk/pull/9984/files - new: https://git.openjdk.org/jdk/pull/9984/files/f57ba86b..0d066921 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9984=02 - incr: https://webrevs.openjdk.org/?repo=jdk=9984=01-02 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/9984.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9984/head:pull/9984 PR: https://git.openjdk.org/jdk/pull/9984
Re: RFR: 8292329: Enable CDS shared heap for zero builds [v2]
On Wed, 24 Aug 2022 10:26:36 GMT, Magnus Ihse Bursie wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @shipilev review comments > > make/Images.gmk line 141: > >> 139: >> 140: $$(eval $$(call SetupExecute, $1_$2_gen_cds_archive_jdk, \ >> 141: WARN := Creating CDS$$($1_$2_DUMP_TYPE) archive for jdk image for >> $1: $$($1_$2_CDS_DUMP_FLAGS), \ > > The actual flags is too much detail to put on the highest (badly named) WARN > level. Instead introduce an additional INFO output, something like this: > > INFO := Using CDS flags for $1: $$($1_$2_CDS_DUMP_FLAGS), \ I've changed it as you suggested. - PR: https://git.openjdk.org/jdk/pull/9984
Re: RFR: 8292329: Enable CDS shared heap for zero builds [v2]
On Tue, 23 Aug 2022 17:12:44 GMT, Erik Joelsson wrote: >> In that case, the loop you proposed make a lot of sense and I think you >> should add it. > > Oh, the loop is already there, it's jus the naming of the variable. I think > we should adjust that so the check-jvm-feature macro can be used. @erikj79 I couldn't get it to work. I believe the following is evaluated only once, so `$(JVM_VARIANT)` is bound to the value when HotspotCommon.gmk is loaded: check-jvm-feature = \ $(strip \ $(if $(filter-out $(VALID_JVM_FEATURES), $1), \ $(error Internal error: Invalid feature tested: $1)) \ $(if $(filter $1, $(JVM_FEATURES_$(JVM_VARIANT))), true, false)) So I would need to run a submake in order to get this function to behave differently for each variant. That's going to be complicated in order to support multiple JVM variants per build. I heard that we are going to remove multiple variant support (?), so maybe this is not worth it. - PR: https://git.openjdk.org/jdk/pull/9984
Re: RFR: 8292329: Enable CDS shared heap for zero builds [v2]
On Tue, 23 Aug 2022 16:56:08 GMT, Erik Joelsson wrote: >> That doesn't work because `check-jvm-feature` requires `JVM_VARIANT` to be >> set, but `CreateCDSArchive` is not called in a context where `JVM_VARIANT` >> is set. ( `JVM_VARIANT` is set only in a few specific places in Main.gmk, >> etc). >> >> One option is to change the foreach loop a few lines below to: >> >> >> $(foreach JVM_VARIANT, $(JVM_VARIANTS), \ >> $(eval $(call CreateCDSArchive,)) \ >> ) >> >> >> But I've not seen `JVM_VARIANT` being used this way, so I am a little >> hesitant about doing it. > > Is the CDS archive dumped in a JVM variant specific way? If so, then it would > make sense to dump it for each configured variant. Yes, it's dumped in a different directory depending on the JVM variant. The contents of the CDS archive is specific to the particular libjvm.so that created the archive. - PR: https://git.openjdk.org/jdk/pull/9984
Re: RFR: 8292329: Enable CDS shared heap for zero builds
On Tue, 23 Aug 2022 16:34:06 GMT, Thomas Stuefe wrote: > Stupid question, does that not mean that the CDS dump generated at build time > is not usable with the VM at runtime if that is started with default options? The CDS archive is still useable. - If G1 is used at runtime, the shared heap is mmaped - If a non-G1 collector is used, all the objects in the shared heap will be copied into the Java heap. See comments near `HeapShared::can_use()`: https://github.com/openjdk/jdk/blob/d24b7b7026cf85f1aecf44f60819762872cfd5c1/src/hotspot/share/cds/heapShared.hpp#L151-L157 - PR: https://git.openjdk.org/jdk/pull/9984
Re: RFR: 8292329: Enable CDS shared heap for zero builds [v2]
On Tue, 23 Aug 2022 16:27:40 GMT, Aleksey Shipilev wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @shipilev review comments > > make/Images.gmk line 132: > >> 130: >> 131: # Only G1 supports dumping the shared heap, so explicitly use G1 >> 132: # it if the JVM supports it. (Note: the default GC with zero is >> SerialGC) > > Suggestion: > > # Only G1 supports dumping the shared heap, so explicitly use G1 if the JVM > supports it. Fixed. > make/Images.gmk line 133: > >> 131: # Only G1 supports dumping the shared heap, so explicitly use G1 >> 132: # it if the JVM supports it. (Note: the default GC with zero is >> SerialGC) >> 133: $1_$2_CDS_DUMP_FLAGS := $(CDS_DUMP_FLAGS) $(if $(filter g1gc, >> $(JVM_FEATURES_$1)),-XX:+UseG1GC) > > I think it should be e.g. (untested): > > Suggestion: > > $1_$2_CDS_DUMP_FLAGS := $(CDS_DUMP_FLAGS) > ifeq ($(call check-jvm-feature, g1gc), true) > $1_$2_CDS_DUMP_FLAGS += -XX:+UseG1GC > endif That doesn't work because `check-jvm-feature` requires `JVM_VARIANT` to be set, but `CreateCDSArchive` is not called in a context where `JVM_VARIANT` is set. ( `JVM_VARIANT` is set only in a few specific places in Main.gmk, etc). One option is to change the foreach loop a few lines below to: $(foreach JVM_VARIANT, $(JVM_VARIANTS), \ $(eval $(call CreateCDSArchive,)) \ ) But I've not seen `JVM_VARIANT` being used this way, so I am a little hesitant about doing it. - PR: https://git.openjdk.org/jdk/pull/9984
Re: RFR: 8292329: Enable CDS shared heap for zero builds [v2]
> ZERO uses UseSerialGC by default. When we dump the default CDS archive during > the build process, it fails to dump the shared heap (which requires G1GC). > > The fix is to force -XX:+UseG1GC when dumping the default CDS archive during > the build process. > > Speed up: > > (Before) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > (After) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.011986 +- 0.000205 seconds time elapsed ( +- 1.71% ) Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @shipilev review comments - Changes: - all: https://git.openjdk.org/jdk/pull/9984/files - new: https://git.openjdk.org/jdk/pull/9984/files/5c490e5e..f57ba86b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9984=01 - incr: https://webrevs.openjdk.org/?repo=jdk=9984=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9984.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9984/head:pull/9984 PR: https://git.openjdk.org/jdk/pull/9984
RFR: 8292329: Enable CDS shared heap for zero builds
ZERO uses UseSerialGC by default. When we dump the default CDS archive during the build process, it fails to dump the shared heap (which requires G1GC). The fix is to force -XX:+UseG1GC when dumping the default CDS archive during the build process. Speed up: (Before) $ perf stat -r 40 ./images/jdk/bin/java -version 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) (After) $ perf stat -r 40 ./images/jdk/bin/java -version 0.011986 +- 0.000205 seconds time elapsed ( +- 1.71% ) - Commit messages: - 8292329: Enable CDS shared heap for zero builds Changes: https://git.openjdk.org/jdk/pull/9984/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9984=00 Issue: https://bugs.openjdk.org/browse/JDK-8292329 Stats: 14 lines in 1 file changed: 10 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/9984.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9984/head:pull/9984 PR: https://git.openjdk.org/jdk/pull/9984
Integrated: 8290981: Enable CDS for zero builds
On Sun, 14 Aug 2022 05:21:13 GMT, Ioi Lam wrote: > Enable CDS for zero builds. `java --version` is about 2x faster now. > > > $ perf stat -r 40 ./images/jdk/bin/java -Xshare:off -version > 0.034645 +- 0.44 seconds time elapsed ( +- 0.13% ) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > I also fixed a bug in Images.gmk that always wrote the default archive to > $JAVA_HOME/lib/server. This fix also makes it possible for a client libjvm to > have a default CDS archive. This pull request has now been integrated. Changeset: 57aac2ab Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/57aac2ab6569c18a56e9a36f174bb0bf09955f83 Stats: 134 lines in 5 files changed: 63 ins; 28 del; 43 mod 8290981: Enable CDS for zero builds Co-authored-by: Aleksey Shipilev Reviewed-by: erikj, shade, ihse - PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v2]
On Tue, 16 Aug 2022 06:36:26 GMT, Aleksey Shipilev wrote: >>> Do you know how much testing is done by the community with zero? We don't >>> test it that much in our pipeline. >> >> You could ask the Debian people (Adrian Glaubitz), IIRC they have test farms >> with broad platform support and use a lot of zero. > >> > Do you know how much testing is done by the community with zero? We don't >> > test it that much in our pipeline. >> >> You could ask the Debian people (Adrian Glaubitz), IIRC they have test farms >> with broad platform support and use a lot of zero. > > Yes, Debian seems to rely on Zero for many arches that do not have Server > ports. They discover the bugs sometimes, but I think it got much better once > we started doing x86_64 Zero bootcycle tests for incoming Zero patches -- it > shakes out a lot of simple bugs. @shipilev Thanks for the code contribution and testing! @tstuefe @magicus @erikj79 Thanks for the review. - PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v4]
On Tue, 16 Aug 2022 16:50:09 GMT, Aleksey Shipilev wrote: >> When running with dynamic dump (`-XX:ArchiveClassesAtExit=foo.jsa`), >> `UseSharedSpaces` is true, so it's possible for a method to be rewritten >> here, and later dumped into the CDS archive). >> >> I think we should remove `!UseSharedSpaces`. I'll try to write a test case >> for it. If I understand correctly, even if `cache->is_vfinal()` is true at >> dump time, it's not guarantee to be true at run time (we might load a >> different version of the class that contains the target method). > > I thought if we run with dynamic dump, then `Arguments::is_dumping_archive()` > is `false`, and we don't rewrite. > > Anyway, if you remove `!UseSharedSpaces` here, like you did in PR update, > then this happens during Linux x86_64 Zero build: > > > $ CONF=linux-x86_64-zero-fastdebug make images > ... > Creating support/classlist.jar You're right. I added the `!UseSharedSpaces` back. I misread the code :-( - PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v6]
> Enable CDS for zero builds. `java --version` is about 2x faster now. > > > $ perf stat -r 40 ./images/jdk/bin/java -Xshare:off -version > 0.034645 +- 0.44 seconds time elapsed ( +- 0.13% ) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > I also fixed a bug in Images.gmk that always wrote the default archive to > $JAVA_HOME/lib/server. This fix also makes it possible for a client libjvm to > have a default CDS archive. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: added !UseSharedSpaces back to _fast_invokevfinal check - Changes: - all: https://git.openjdk.org/jdk/pull/9869/files - new: https://git.openjdk.org/jdk/pull/9869/files/eb0e295d..8e4bef9d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9869=05 - incr: https://webrevs.openjdk.org/?repo=jdk=9869=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9869.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9869/head:pull/9869 PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v4]
On Tue, 16 Aug 2022 06:31:41 GMT, Aleksey Shipilev wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed copyright and whitespaces
>
> src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 2455:
>
>> 2453: if (cache->is_vfinal()) {
>> 2454: callee = cache->f2_as_vfinal_method();
>> 2455: if (REWRITE_BYTECODES && !UseSharedSpaces &&
>> !Arguments::is_dumping_archive()) {
>
> As we discussed with @tstuefe in another thread, the use of `UseSharedSpaces`
> needs the explicit `#include` of `globals.hpp`.
When running with dynamic dump (`-XX:ArchiveClassesAtExit=foo.jsa`),
`UseSharedSpaces` is true, so it's possible for a method to be rewritten here,
and later dumped into the CDS archive).
I think we should remove `!UseSharedSpaces`. I'll try to write a test case for
it. If I understand correctly, even if `cache->is_vfinal()` is true at dump
time, it's not guarantee to be true at run time (we might load a different
version of the class that contains the target method).
> test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveNoDefaultArchive.java
> line 108:
>
>> 106: removeDefaultArchives(java_home_dst, "zero");
>> 107: removeDefaultArchives(java_home_dst, "server");
>> 108: removeDefaultArchives(java_home_dst, "zero");
>
> What is the point of doing "zero" part twice?
Fixed. It should be "client".
-
PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v5]
> Enable CDS for zero builds. `java --version` is about 2x faster now. > > > $ perf stat -r 40 ./images/jdk/bin/java -Xshare:off -version > 0.034645 +- 0.44 seconds time elapsed ( +- 0.13% ) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > I also fixed a bug in Images.gmk that always wrote the default archive to > $JAVA_HOME/lib/server. This fix also makes it possible for a client libjvm to > have a default CDS archive. Ioi Lam has updated the pull request incrementally with two additional commits since the last revision: - fixed typo in test - also disable _fast_invokevfinal for dynamic dump - Changes: - all: https://git.openjdk.org/jdk/pull/9869/files - new: https://git.openjdk.org/jdk/pull/9869/files/94fd577e..eb0e295d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9869=04 - incr: https://webrevs.openjdk.org/?repo=jdk=9869=03-04 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/9869.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9869/head:pull/9869 PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v2]
On Tue, 16 Aug 2022 06:36:26 GMT, Aleksey Shipilev wrote: > > Do you know how much testing is done by the community with zero? We don't > > test it that much in our pipeline. > > You could ask the Debian people (Adrian Glaubitz), IIRC they have test farms > with broad platform support and use a lot of zero. @glaubitz, could you try out this PR and see if it might introduce any regressions? I'd really appreciate your help. - PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v4]
> Enable CDS for zero builds. `java --version` is about 2x faster now. > > > $ perf stat -r 40 ./images/jdk/bin/java -Xshare:off -version > 0.034645 +- 0.44 seconds time elapsed ( +- 0.13% ) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > I also fixed a bug in Images.gmk that always wrote the default archive to > $JAVA_HOME/lib/server. This fix also makes it possible for a client libjvm to > have a default CDS archive. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: fixed copyright and whitespaces - Changes: - all: https://git.openjdk.org/jdk/pull/9869/files - new: https://git.openjdk.org/jdk/pull/9869/files/2716becb..94fd577e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9869=03 - incr: https://webrevs.openjdk.org/?repo=jdk=9869=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/9869.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9869/head:pull/9869 PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v2]
On Mon, 15 Aug 2022 17:18:54 GMT, Aleksey Shipilev wrote: >>> Good stuff. Actually, let me see how easy it is to implement the `nofast_*` >>> bytecodes in Zero, so we don't have to do the shared-code exceptions for it. >> >> That took some debugging :) But here it is, applied on top of your PR: >> https://cr.openjdk.java.net/~shade/8290981/zero-cds-nofast-1.patch -- builds >> Linux x86_64 Zero fine, this includes using the Zero VM to compile parts of >> JDK. I can do more testing later, we should at least test it bootcycles well. > >> > Good stuff. Actually, let me see how easy it is to implement the >> > `nofast_*` bytecodes in Zero, so we don't have to do the shared-code >> > exceptions for it. >> >> That took some debugging :) But here it is, applied on top of your PR: >> https://cr.openjdk.java.net/~shade/8290981/zero-cds-nofast-1.patch -- builds >> Linux x86_64 Zero fine, this includes using the Zero VM to compile parts of >> JDK. I can do more testing later, we should at least test it bootcycles well. > > Passes Linux x86_64 fastdebug Zero `make bootcycle-images` too. @shipilev thanks for sending the patch. I've integrated it. I ran all the tests under test/hotspot/jtreg/runtime/cds/appcds. They all passed after I fixed two of the test cases. Do you know how much testing is done by the community with zero? We don't test it that much in our pipeline. - PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v3]
> Enable CDS for zero builds. `java --version` is about 2x faster now. > > > $ perf stat -r 40 ./images/jdk/bin/java -Xshare:off -version > 0.034645 +- 0.44 seconds time elapsed ( +- 0.13% ) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > I also fixed a bug in Images.gmk that always wrote the default archive to > $JAVA_HOME/lib/server. This fix also makes it possible for a client libjvm to > have a default CDS archive. 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 seven additional commits since the last revision: - @erikj79 comments; also fixed CDS test cases for zero - Merge branch 'master' into 8290981-enable-cds-for-zero - imported contribution by @shipilev - https://cr.openjdk.java.net/~shade/8290981/zero-cds-nofast-1.patch - create_cds_archive -> CreateCDSArchive - explicitly choose VM variant in create_cds_archive - clean up C code - 8290981: Enable CDS for zero builds - Changes: - all: https://git.openjdk.org/jdk/pull/9869/files - new: https://git.openjdk.org/jdk/pull/9869/files/004c5038..2716becb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9869=02 - incr: https://webrevs.openjdk.org/?repo=jdk=9869=01-02 Stats: 20828 lines in 932 files changed: 11978 ins; 5521 del; 3329 mod Patch: https://git.openjdk.org/jdk/pull/9869.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9869/head:pull/9869 PR: https://git.openjdk.org/jdk/pull/9869
Re: RFR: 8290981: Enable CDS for zero builds [v2]
> Enable CDS for zero builds. `java --version` is about 2x faster now. > > > $ perf stat -r 40 ./images/jdk/bin/java -Xshare:off -version > 0.034645 +- 0.44 seconds time elapsed ( +- 0.13% ) > $ perf stat -r 40 ./images/jdk/bin/java -version > 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) > > I also fixed a bug in Images.gmk that always wrote the default archive to > $JAVA_HOME/lib/server. This fix also makes it possible for a client libjvm to > have a default CDS archive. Ioi Lam has updated the pull request incrementally with two additional commits since the last revision: - create_cds_archive -> CreateCDSArchive - explicitly choose VM variant in create_cds_archive - Changes: - all: https://git.openjdk.org/jdk/pull/9869/files - new: https://git.openjdk.org/jdk/pull/9869/files/4d44c048..004c5038 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9869=01 - incr: https://webrevs.openjdk.org/?repo=jdk=9869=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/9869.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9869/head:pull/9869 PR: https://git.openjdk.org/jdk/pull/9869
RFR: 8290981: Enable CDS for zero builds
Enable CDS for zero builds. `java --version` is about 2x faster now. $ perf stat -r 40 ./images/jdk/bin/java -Xshare:off -version 0.034645 +- 0.44 seconds time elapsed ( +- 0.13% ) $ perf stat -r 40 ./images/jdk/bin/java -version 0.018080 +- 0.000388 seconds time elapsed ( +- 2.15% ) I also fixed a bug in Images.gmk that always wrote the default archive to $JAVA_HOME/lib/server. This fix also makes it possible for a client libjvm to have a default CDS archive. - Commit messages: - clean up C code - 8290981: Enable CDS for zero builds Changes: https://git.openjdk.org/jdk/pull/9869/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9869=00 Issue: https://bugs.openjdk.org/browse/JDK-8290981 Stats: 85 lines in 5 files changed: 32 ins; 25 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/9869.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9869/head:pull/9869 PR: https://git.openjdk.org/jdk/pull/9869
"make hotspot" builds test .obj files first
My windows build is very slow, and it spends the first few minutes compiling test_xxx.obj files. Is it possible to build the HotSpot VM .obj files first? Thanks - Ioi $ time make hotspot LOG=info Generating main target list Running make as 'make LOG=info hotspot' Building target 'hotspot' in configuration 'windows-x64-slowdebug' Building JVM variant 'server' with features 'cds compiler1 compiler2 epsilongc g1gc jfr jni-check jvmci jvmti management oracle-src parallelgc serialgc services vm-structs zgc' Updating support/modules_libs/java.base/server/jvm.dll due to 6 file(s) Compiling BUILD_GTEST_LIBJVM_pch.cpp (for jvm.dll) Compiling BUILD_LIBJVM_pch.cpp (for jvm.dll) Compiling gtestMain.cpp (for jvm.dll) Compiling logTestFixture.cpp (for jvm.dll) Compiling metaspaceGtestCommon.cpp (for jvm.dll) Compiling metaspaceGtestContexts.cpp (for jvm.dll) Compiling test_AltHashing.cpp (for jvm.dll) Compiling test_ThreadsListHandle.cpp (for jvm.dll) Compiling test_adaptiveSampler.cpp (for jvm.dll) Compiling test_align.cpp (for jvm.dll) Compiling test_allocationGuard.cpp (for jvm.dll) Compiling test_arena.cpp (for jvm.dll) Compiling test_arenagrowthpolicy.cpp (for jvm.dll)
