Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v15]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: Missed one file from last commit - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/02ac6d6e..22c47d33 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=13-14 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v14]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - Added more exclusions to hotspot_aot_classlinking test group, as these tests disable full-module-graph - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking - @dholmes-ora comments - @dholmes-ora comments - Fixed ZERO build - minor comment fix - @ashu-mehra comment: move code outside of call_initPhase2(); also renamed BOOT/BOOT2 to BOOT1/BOOT2 and refactored code related to AOTLinkedClassCategory - @ashu-mehra reviews - ... and 8 more: https://git.openjdk.org/jdk/compare/c068db8e...02ac6d6e - Changes: https://git.openjdk.org/jdk/pull/20843/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=13 Stats: 1795 lines in 47 files changed: 1638 ins; 57 del; 100 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v13]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits: - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking - @dholmes-ora comments - @dholmes-ora comments - Fixed ZERO build - minor comment fix - @ashu-mehra comment: move code outside of call_initPhase2(); also renamed BOOT/BOOT2 to BOOT1/BOOT2 and refactored code related to AOTLinkedClassCategory - @ashu-mehra reviews - @ashu-mehra comments - @adinn comments - ... and 6 more: https://git.openjdk.org/jdk/compare/0859631f...3cdc7634 - Changes: https://git.openjdk.org/jdk/pull/20843/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=12 Stats: 1788 lines in 47 files changed: 1631 ins; 57 del; 100 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v11]
On Thu, 19 Sep 2024 05:37:17 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dholmes-ora comments
>
> src/hotspot/share/cds/aotClassLinker.cpp line 122:
>
>> 120: assert(CDSConfig::is_dumping_aot_linked_classes(), "sanity");
>> 121:
>> 122: if (!SystemDictionaryShared::is_builtin(ik)) {
>
> What does this actually mean by "built-in"?
Boot/Platform/App loaders. The meaning of "built-in" is documented at the top
of systemDictionaryShared.hpp
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1771839512
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v12]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking - @dholmes-ora comments - @dholmes-ora comments - Fixed ZERO build - minor comment fix - @ashu-mehra comment: move code outside of call_initPhase2(); also renamed BOOT/BOOT2 to BOOT1/BOOT2 and refactored code related to AOTLinkedClassCategory - @ashu-mehra reviews - @ashu-mehra comments - @adinn comments - @dholmes-ora comments: logging indents - ... and 5 more: https://git.openjdk.org/jdk/compare/49dbfa6a...6029b35f - Changes: https://git.openjdk.org/jdk/pull/20843/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=11 Stats: 1787 lines in 47 files changed: 1630 ins; 57 del; 100 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v9]
On Wed, 18 Sep 2024 05:01:00 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed ZERO build
>
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 66:
>
>> 64:
>> 65: void AOTLinkedClassBulkLoader::load_classes_in_loader(JavaThread*
>> current, AOTLinkedClassCategory class_category, oop class_loader_oop) {
>> 66: ExceptionMark em(current);
>
> Why do you need the EM when you are explicitly checking for exceptions?
Removed.
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 67:
>
>> 65: void AOTLinkedClassBulkLoader::load_classes_in_loader(JavaThread*
>> current, AOTLinkedClassCategory class_category, oop class_loader_oop) {
>> 66: ExceptionMark em(current);
>> 67: ResourceMark rm(current);
>
> The RM should go where it is actually needed for the logging.
Fixed
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 68:
>
>> 66: ExceptionMark em(current);
>> 67: ResourceMark rm(current);
>> 68: HandleMark hm(current);
>
> Why do you need a HM here?
It was needed when this code was called from a different path. It's no longer
needed now, so I removed it.
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 95:
>
>> 93:
>> 94: if (Universe::is_fully_initialized() && VerifyDuringStartup) {
>> 95: // Make sure we're still in a clean slate.
>
> Suggestion:
>
> // Make sure we're still in a clean state.
Fixed
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 132:
>
>> 130: break;
>> 131: case AOTLinkedClassCategory::UNREGISTERED:
>> 132: ShouldNotReachHere(); // Currently aot-linked classes are not
>> supported for this category.
>
> Suggestion:
>
> case AOTLinkedClassCategory::UNREGISTERED:
> default:
> ShouldNotReachHere(); // Currently aot-linked classes are not supported
> for this category.
Fixed.
> src/hotspot/share/cds/aotLinkedClassTable.hpp line 34:
>
>> 32: class SerializeClosure;
>> 33:
>> 34: // Classes to be buik-loaded, in the "linked" state, at VM bootstrap.
>
> Suggestion:
>
> // Classes to be bulk-loaded, in the "linked" state, at VM bootstrap.
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121555
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121383
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121348
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121642
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121689
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121855
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v8]
On Wed, 18 Sep 2024 01:56:36 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> minor comment fix
>
> src/hotspot/share/cds/aotClassLinker.cpp line 212:
>
>> 210: } else {
>> 211: const char* category = class_category_name(list.at(0));
>> 212: log_info(cds, aot, link)("written %d class(es) for category %s",
>> list.length(), category);
>
> Suggestion:
>
> log_info(cds, aot, link)("wrote %d class(es) for category %s",
> list.length(), category);
Fixed
> src/hotspot/share/cds/aotClassLinker.hpp line 100:
>
>> 98: static bool is_vm_class(InstanceKlass* ik);
>> 99:
>> 100: // When CDS is enabled, is ik guatanteed to be linked at deployment
>> time (and
>
> Suggestion:
>
> // When CDS is enabled, is ik guaranteed to be linked at deployment time
> (and
Fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121912
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121483
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v8]
On Wed, 18 Sep 2024 01:59:59 GMT, David Holmes wrote: >> src/hotspot/share/cds/aotClassLinker.hpp line 60: >> >>> 58: // - The visibility of C >>> 59: // >>> 60: // During an Production Run, the JVM can use an AOTCache with an >>> AOTLinkedClassTable >> >> Suggestion: >> >> // During a Production Run, the JVM can use an AOTCache with an >> AOTLinkedClassTable > > Why is "Production Run" capitalized? Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121446
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v11]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @dholmes-ora comments - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/3215c002..dd5a5ba6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=09-10 Stats: 15 lines in 4 files changed: 2 ins; 8 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v9]
On Wed, 18 Sep 2024 05:07:33 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed ZERO build
>
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 170:
>
>> 168: log_error(cds)("Unable to resolve %s class from CDS archive:
>> %s", category_name, ik->external_name());
>> 169: log_error(cds)("Expected: " INTPTR_FORMAT ", actual: "
>> INTPTR_FORMAT, p2i(ik), p2i(actual));
>> 170: log_error(cds)("JVMTI class retransformation is not supported
>> when archive was generated with -XX:+AOTClassLinking.");
>
> Nit: use a `logStream` instead of the three separate calls.
Why?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766119067
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v9]
On Wed, 18 Sep 2024 05:11:48 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed ZERO build
>
> src/hotspot/share/cds/archiveBuilder.cpp line 316:
>
>> 314:
>> 315: if (CDSConfig::is_dumping_aot_linked_classes()) {
>> 316: _estimated_hashtable_bytes += _klasses->length() * 16 *
>> sizeof(Klass*);
>
> Why 16?
Doing the estimate is actually difficult here (and also pointless). I've filed
https://bugs.openjdk.org/browse/JDK-8340416 to remove the estimation
altogether. For the time being, I change the estimate to 20MB which will be
more than enough.
> src/hotspot/share/cds/archiveBuilder.cpp line 877:
>
>> 875: if (ik->is_hidden()) {
>> 876: ADD_COUNT(num_hidden_klasses);
>> 877: hidden = " hidden";
>
> Why not do this at the same time you do the other hidden class updates above?
I moved the code.
> src/hotspot/share/cds/cds_globals.hpp line 99:
>
>> 97:
>>\
>> 98: /*== New "AOT" flags
>> =*/ \
>> 99: /* The following 3 flags are aliases of -Xshare:dump, */
>>\
>
> Nit: align the `*/`.
Fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766116185
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766116232
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766116258
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v8]
On Wed, 18 Sep 2024 01:47:26 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> minor comment fix
>
> src/hotspot/share/cds/aotClassLinker.cpp line 121:
>
>> 119: assert(is_initialized(), "sanity");
>> 120:
>> 121: if (!CDSConfig::is_dumping_aot_linked_classes() ||
>> !SystemDictionaryShared::is_builtin(ik)) {
>
> Shouldn't the CDSConfig check just be an assert - the caller is expected to
> check before trying to add candidates?
Fixed
> src/hotspot/share/cds/aotClassLinker.cpp line 145:
>
>> 143: return false;
>> 144: }
>> 145: }
>
> Are we concerned with the possibility that we might be able to add some
> interfaces but not all, hence returning false, but with a subset of
> interfaces already added to the candidate list? I don't think it should be
> possible, but the code structure makes it look like it could be possible.
That's possible. Since we go through all the initial set of classes found in
`ArchiveBuilder::current()->klasses()`, eventually all classes that are
eligible for aot-linking will be added to the candidate list. The reason for
the recursion is to
- Filter out classes whose super types are not eligible
- Sort the candidate list so that super types always come before sub types.
> src/hotspot/share/cds/aotClassLinker.cpp line 191:
>
>> 189: if (ik->class_loader() != class_loader) {
>> 190: continue;
>> 191: }
>
> This seems very inefficient. We call `write_classes` 4 times, potentially
> with different loaders. Because the candidates are sorted the classes
> belonging to the same loader are likely to be grouped due to package names.
> So the app loader classes are likely to be right at end, and we have to
> traverse all the boot/platform classes first before we get to them.
> Conversely after we have encountered the last boot loader class (for example)
> we keep the scanning the entire list. If the set were ordered based on loader
> then name, we would be able to stop once we see the loader change to not
> being the desired one. And a binaery search would let you find the start of a
> section more quickly.
The classes are sorted by class hierarchy. It's a linear operation repeated 4
times, so it's not really something we need to optimize.
> src/hotspot/share/cds/aotClassLinker.cpp line 194:
>
>> 192: if ((ik->module() == ModuleEntryTable::javabase_moduleEntry()) !=
>> is_javabase) {
>> 193: continue;
>> 194: }
>
> Why do we process system loader classes (i.e. application loader classes) if
> they need to be in java.base, as the application classes will never be in
> java.base. ???
The code is written for simplicity. If it becomes a performance problem we can
change it.
> src/hotspot/share/cds/aotClassLinker.cpp line 198:
>
>> 196: if (ik->is_shared() && CDSConfig::is_dumping_dynamic_archive()) {
>> 197: if (CDSConfig::is_using_aot_linked_classes()) {
>> 198: // This class was recorded as a AOT-linked for the base archive,
>
> Suggestion:
>
> // This class was recorded as AOT-linked for the base archive,
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115216
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115133
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115349
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115433
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766115463
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v10]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @dholmes-ora comments - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/be1d0ef1..3215c002 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=08-09 Stats: 16 lines in 3 files changed: 8 ins; 4 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v9]
On Wed, 18 Sep 2024 05:35:42 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed ZERO build
>
> test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/AOTClassLinkingVMOptions.java
> line 57:
>
>> 55: testCase("Archived full module graph must be enabled at
>> runtime");
>> 56: TestCommon.run("-cp", appJar, "-Djdk.module.validation=1",
>> "Hello")
>> 57: .assertAbnormalExit("CDS archive has aot-linked classes." +
>
> Nit: align the dots
The CDS test cases indent by 4 spaces in this situation. I searched for `'^
*[.]'` lines in the JDK source code and indentation of 8 and 4 spaces seem to
be most common.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766089235
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v9]
On Wed, 18 Sep 2024 05:33:09 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed ZERO build
>
> src/hotspot/share/classfile/systemDictionary.cpp line 139:
>
>> 137: if (_java_platform_loader.is_empty()) {
>> 138: oop platform_loader = get_platform_class_loader_impl(CHECK);
>> 139: _java_platform_loader = OopHandle(Universe::vm_global(),
>> platform_loader);
>
> Why has the order been switched here?
It's just clean up. Platform loader should go before app loader.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766082395
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v9]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: Fixed ZERO build - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/bedf9a26..be1d0ef1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=07-08 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v6]
On Tue, 17 Sep 2024 19:52:29 GMT, Ashutosh Mehra wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @ashu-mehra reviews
>
> src/hotspot/share/runtime/threads.cpp line 322:
>
>> 320: universe_post_module_init();
>> 321:
>> 322: if (CDSConfig::is_using_aot_linked_classes()) {
>
> call_initPhase2 has a timer that computes cost for initializing module
> system. Before this patch call_initPhase2 was only initializing the module
> system. But now it is doing work which is not part of the module system
> initialization. So probably in future we may want to refactor this work out
> of call_initPhase2.
Hi @ashu-mehra , I moved the code outside of `call_initPhase2()`, and
consolidated it into `AOTLinkedClassBulkLoader::load_non_javabase_classes()`.
While doing that, I noticed that the enums `BOOT` and `BOOT2` are misleading --
it seems the former would be a superset of the latter, but in fact they are
disjoint. I think this is one reason that @dholmes-ora wanted different names.
So I renamed them to `BOOT1` vs `BOOT2` (boot classes loaded in 1st vs 2nd
phase). I also renamed the enum to AOTLinkedClassCategory and refactored the
related code. There's a lot of renaming but the logic is unchanged.
Ashu and David, please re-review
https://github.com/openjdk/jdk/pull/20843/commits/4e9668a0b85fd5aa5839528e30c2955b424ac8ca
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764211572
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v8]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: minor comment fix - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/4e9668a0..bedf9a26 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=06-07 Stats: 4 lines in 2 files changed: 1 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v7]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @ashu-mehra comment: move code outside of call_initPhase2(); also renamed BOOT/BOOT2 to BOOT1/BOOT2 and refactored code related to AOTLinkedClassCategory - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/bcddf963..4e9668a0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=05-06 Stats: 194 lines in 8 files changed: 80 ins; 67 del; 47 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v5]
On Fri, 13 Sep 2024 16:09:25 GMT, Ashutosh Mehra wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @ashu-mehra comments
>
> src/hotspot/share/cds/aotClassLinker.cpp line 227:
>
>> 225: }
>> 226:
>> 227: int AOTClassLinker::num_initiated_classes(oop loader1, oop loader2) {
>
> The two loader arguments here are quite confusing marking it hard to
> understand the code. Can it be refactored as this:
>
>
> int AOTClassLinker::num_platform_initiated_classes() {
> // AOTLinkedClassBulkLoader will initiate loading of all public boot
> classes in the platform loader.
> return num_initiated_classes(nullptr);
> }
>
> int AOTClassLinker::num_app_initiated_classes() {
> // AOTLinkedClassBulkLoader will initiate loading of all public
> boot/platform classes in the app loader.
> return num_platform_initiated_classes +
> num_initiated_classes(SystemDictionary::java_platform_loader());
> }
>
> int AOTClassLinker::num_initiated_classes(oop loader) {
> int n = 0;
> for (int i = 0; i < _sorted_candidates->length(); i++) {
> InstanceKlass* ik = _sorted_candidates->at(i);
> if (ik->is_public() && !ik->is_hidden() &&
> (ik->class_loader() == loader) {
> n++;
> }
> }
>
> return n;
> }
I renamed the function to `AOTClassLinker::count_public_classes()` and made it
handle only a single loader each time. This function is not speed critical so I
think this way it's much easier to read.
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 199:
>
>> 197: InstanceKlass* ik = classes->at(i);
>> 198: assert(ik->is_loaded(), "must have already been loaded by a parent
>> loader");
>> 199: assert(ik->class_loader() != initiating_loader(), "must be a parent
>> loader");
>
> Can we also add an assert that ik->class_loader() must be either boot or
> platform loader.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1762007825
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1762007881
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v6]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @ashu-mehra reviews - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/66a4ff41..bcddf963 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=04-05 Stats: 18 lines in 3 files changed: 10 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v5]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @ashu-mehra comments - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/5bba4ad4..66a4ff41 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=03-04 Stats: 28 lines in 6 files changed: 11 ins; 0 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v3]
On Tue, 10 Sep 2024 21:31:31 GMT, Ashutosh Mehra wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dholmes-ora comments: logging indents
>
> src/hotspot/share/cds/aotClassLinker.cpp line 149:
>
>> 147: add_candidate(ik);
>> 148:
>> 149: if (log_is_enabled(Info, cds, aot, load)) {
>
> Is `load` the correct log tag to use in this class? Can it be replaced with
> `link` tag?
I changed to the `link` tag.
> src/hotspot/share/cds/aotClassLinker.hpp line 111:
>
>> 109:
>> 110: static int num_app_initiated_classes();
>> 111: static int num_platform_initiated_classes();
>
> I don't see these methods (num_app_initiated_classes and
> num_platform_initiated_classes) used anywhere. Should they be removed?
I added logging in DumpAllocStats::print_stats() to use these methods.
> src/hotspot/share/cds/aotConstantPoolResolver.cpp line 111:
>
>> 109:
>> 110: if (CDSConfig::is_dumping_aot_linked_classes()) {
>> 111: if (AOTClassLinker::try_add_candidate(ik)) {
>
> Are we relying on the call to `try_add_candidate` to add the class to the
> candidate list? I guess that shouldn't be the case as the class have already
> been added through
> ArchiveBuilder::gather_klasses_and_symbols()->AOTClassLinker::add_candidates().
> If so can we use AOTClassLinker::is_candidate(ik) here?
You're correct. I changed it to `AOTClassLinker::is_candidate(ik)`
> src/hotspot/share/cds/archiveBuilder.cpp line 766:
>
>> 764: #define ADD_COUNT(x) \
>> 765: x += 1; \
>> 766: x ## _a += aotlinked;
>
> Can we do this instead:
>
> ```x ## _a += (aotlinked ? 1 : 0)```
>
> and make `aotlinked` a bool.
Fixed.
> src/hotspot/share/cds/archiveBuilder.cpp line 779:
>
>> 777: DECLARE_INSTANCE_KLASS_COUNTER(num_app_klasses);
>> 778: DECLARE_INSTANCE_KLASS_COUNTER(num_hidden_klasses);
>> 779: DECLARE_INSTANCE_KLASS_COUNTER(num_unlinked_klasses);
>
> Nit-picking here - "unlinked" category doesn't need the "aot-linked" counter.
Fixed.
> src/hotspot/share/cds/filemap.cpp line 2455:
>
>> 2453: const char* prop =
>> Arguments::get_property("java.system.class.loader");
>> 2454: if (prop != nullptr) {
>> 2455: if (has_aot_linked_classes()) {
>
> Should this check be part of `FileMapInfo::validate_aot_class_linking`?
I put the check here so that the "Archived non-system classes are disabled
because " message will not be printed a few lines below. Otherwise the user
will see two different error messages that say different things.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622677
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622615
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622522
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622803
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622734
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622394
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v4]
On Thu, 12 Sep 2024 14:04:40 GMT, Ashutosh Mehra wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @adinn comments
>
> test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java line
> 110:
>
>> 108: "" + ClassFileLoadHook.TestCaseId.SHARING_ON_CFLH_ON);
>> 109: if (out.contains("Using AOT-linked classes: false")) {
>> 110: // We are running with VM options that do not support
>> -XX:+AOTClassLinking
>
> When will we run into this case? Is there a VM option that would silently
> disable AOTClassLinking in prod run?
AOTClassLinking requires the ability to write the full archived module graph
(FMG), so it will be disabled if jtreg is executed with
- a GC that doesn't support object heap writing, such as ZGC
- a module related option that's not compatible with FMG, such as --add-opens
I edited the comment in the test to make it clear that the JVM options affects
the writing of the archive. I also modified the log to something like this:
Using AOT-linked classes: false (static archive: no aot-linked classes)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r175768
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v3]
On Tue, 10 Sep 2024 10:08:35 GMT, Andrew Dinn wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dholmes-ora comments: logging indents
>
> src/hotspot/share/cds/aotClassLinker.hpp line 71:
>
>> 69: //
>> 70: class AOTClassLinker : AllStatic {
>> 71: using ClassesTable = ResourceHashtable> AnyObj::C_HEAP, mtClassShared>;
>
> Can we have a symbolic name for this (prime) magic number here and in other
> places in this patch? I realise there is existing code which uses the raw
> number bit it is also consumed symbolically (e.g. in archiveBuilder.hpp,
> metaspaceClosure.hpp)
I added a `static const int TABLE_SIZE = 15889; // prime number`
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 191:
>
>> 189: }
>> 190:
>> 191: ClassLoaderData* loader_data =
>> ClassLoaderData::class_loader_data(loader());
>
> Can we assert here that loader() != nullptr?
I added a few asserts in this function about the expected initiating and
defining loaders.
> src/hotspot/share/cds/archiveBuilder.cpp line 433:
>
>> 431: }
>> 432:
>> 433: remember_embedded_pointer_in_enclosing_obj(ref);
>
> I'm not clear why this was moved up. Was this just an omission (bug) in the
> earlier version or do we now need to remember a reference location that we
> could previously safely ignore?
If I remember correctly, this is a latent bug where we didn't remember the
embedded pointer when the function returns early. This led to a crash that
happened only when -XX:+AOTClassLinking is enabled.
> src/hotspot/share/cds/cdsConfig.cpp line 551:
>
>> 549: }
>> 550:
>> 551: void CDSConfig::set_has_aot_linked_classes(bool is_static_archive, bool
>> has_aot_linked_classes) {
>
> Why does this need to take `is_static_archive` as an argument?
This argument is unused and I removed it.
> src/hotspot/share/cds/dynamicArchive.cpp line 138:
>
>> 136: verify_estimate_size(_estimated_metaspaceobj_bytes,
>> "MetaspaceObjs");
>> 137:
>> 138: sort_methods();
>
> Could we have a comment to note that sorting and making shareable need to be
> done before calling `AOTClassLinker::write_to_archive();`
The reason I moved this code is I though it just looks odd -- we write a bunch
of meta information about the classes, and then proceed on changing the
contents of the classes.
The new order -- fix up the classes and then write the meta info -- should be
quite natural, so I think a comment isn't needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755536629
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755536786
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755536983
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755537234
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1755537319
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v4]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @adinn comments - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/0441aef0..5bba4ad4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=02-03 Stats: 16 lines in 7 files changed: 5 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v2]
On Mon, 9 Sep 2024 05:15:20 GMT, David Holmes wrote: >> I tried to separate the "types" from the "values". I think this makes it >> easy to see how many types there are. > > Sorry I don't follow. This is just like a printf call I changed it according to your suggestion. - PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1751101001
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v3]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @dholmes-ora comments: logging indents - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/ac1ed798..0441aef0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v2]
On Sun, 8 Sep 2024 23:46:03 GMT, David Holmes wrote: >> I prefer boot/boot2 to make the output easier to read. Anyone debugging this >> output will need to read the code to understand what "boot2" or >> "boot-nonbase" is. A few extra characters here will not help. > > Sorry but '2' conveys zero information whereas 'nonbase' tells you they are > not in the base module. It's the second group of boot loaders classes. - PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1749425914
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v2]
On Sun, 8 Sep 2024 23:49:45 GMT, David Holmes wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @dholmes-ora comments > > src/hotspot/share/cds/archiveBuilder.cpp line 912: > >> 910: >> STATS_PARAMS(unlinked_klasses), >> 911: boot_unlinked, >> platform_unlinked, >> 912: app_unlinked, unreg_unlinked); > > This indentation still looks weird. Arguments on newlines should be aligned. I tried to separate the "types" from the "values". I think this makes it easy to see how many types there are. - PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1749426533
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v2]
On Fri, 6 Sep 2024 05:24:18 GMT, David Holmes wrote: > I've taken an initial look through but there is an awful lot to try and > digest here. I've flagged numerous typos and minor nits. > > One general query: does this stuff work if the user defines their own initial > application classloader? Hi David thanks for the review. I've pushed a new version that has most of your suggestions. I also added code to avoid loading the CDS archive if it has aot-linked classes, and the user has specified `-Djava.system.class.loader` > src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 86: > >> 84: >> 85: load_table(AOTLinkedClassTable::for_static_archive(), loader_kind, >> h_loader, current); >> 86: assert(!current->has_pending_exception(), "VM should have exited due >> to ExceptionMark"); > > An `ExceptionMark` only triggers a VM exit on construction and destruction, > if an exception is pending. I refactored the code a bit to handle any exception that occurs during bulk loading, and print our more useful error messages. - PR Comment: https://git.openjdk.org/jdk/pull/20843#issuecomment-2334966941 PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1747837592
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v2]
> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > **Overview** > > - A new `-XX:+AOTClassLinking` flag is added. See [JEP > 498](https://bugs.openjdk.org/browse/JDK-8315737) and the > [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this > command-line option, its default value, and its impact on compatibility. > - When this flag is enabled during the creation of an AOT cache (aka CDS > archive), an `AOTLinkedClassTable` is added to the cache to include all > classes that are AOT-linked. For this PR, only classes for the > boot/platform/application loaders are eligible. The main logic is in > `aotClassLinker.cpp`. > - When an AOT archive is loaded in a production run, all classes in the > `AOTLinkedClassTable` are loaded into their respective class loaders at the > earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. > - The boot classes are loaded as part of `vmClasses::resolve_all()` > - The platform/application classes are loaded after the module graph is > restored (see changes in `threads.cpp`). > - Since all classes in a `AOTLinkedClassTable` are loaded before any > user-code is executed, we can resolve constant pool entries that refer to > these classes during AOT cache creation. See changes in > `AOTConstantPoolResolver::is_class_resolution_deterministic()`. > > **All-or-nothing Loading** > > - Because AOT-linked classes can refer to each other, using direct C++ > pointers, all AOT-linked classes must be loaded together. Otherwise we will > have dangling C++ pointers in the class metadata structures. > - During a production run, we check during VM start-up for incompatible VM > options that would prevent some of the AOT-linked classes from being loaded. > For example: > - If the VM is started with an JVMTI agent that has ClassFileLoadHook > capabilities, it could replace some of the AOT-linked classes with > alternative versions. > - If the VM is started with certain module options, such as > `--patch-module` or `--module`, some AOT-linked classes may be replaced with > patched versions, or may become invisible and cannot be loaded into the JVM. > - When incompatible VM options are detected, the JVM will refuse to load an > AOT cache that has AOT-linked classes. See > `FileMapInfo::validate_aot_class_linking()`. > - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires > `CDSConfig::is_using_full_module_graph()` to be true. This means that the > exact same set of modules are visible whe... Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @dholmes-ora comments - Changes: - all: https://git.openjdk.org/jdk/pull/20843/files - new: https://git.openjdk.org/jdk/pull/20843/files/a5e7eb51..ac1ed798 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=00-01 Stats: 61 lines in 7 files changed: 29 ins; 15 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8329706: Implement -XX:+AOTClassLinking
On Fri, 6 Sep 2024 05:10:42 GMT, David Holmes wrote:
>> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>>
>> **Overview**
>>
>> - A new `-XX:+AOTClassLinking` flag is added. See [JEP
>> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the
>> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this
>> command-line option, its default value, and its impact on compatibility.
>> - When this flag is enabled during the creation of an AOT cache (aka CDS
>> archive), an `AOTLinkedClassTable` is added to the cache to include all
>> classes that are AOT-linked. For this PR, only classes for the
>> boot/platform/application loaders are eligible. The main logic is in
>> `aotClassLinker.cpp`.
>> - When an AOT archive is loaded in a production run, all classes in the
>> `AOTLinkedClassTable` are loaded into their respective class loaders at the
>> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`.
>> - The boot classes are loaded as part of `vmClasses::resolve_all()`
>> - The platform/application classes are loaded after the module graph is
>> restored (see changes in `threads.cpp`).
>> - Since all classes in a `AOTLinkedClassTable` are loaded before any
>> user-code is executed, we can resolve constant pool entries that refer to
>> these classes during AOT cache creation. See changes in
>> `AOTConstantPoolResolver::is_class_resolution_deterministic()`.
>>
>> **All-or-nothing Loading**
>>
>> - Because AOT-linked classes can refer to each other, using direct C++
>> pointers, all AOT-linked classes must be loaded together. Otherwise we will
>> have dangling C++ pointers in the class metadata structures.
>> - During a production run, we check during VM start-up for incompatible VM
>> options that would prevent some of the AOT-linked classes from being loaded.
>> For example:
>> - If the VM is started with an JVMTI agent that has ClassFileLoadHook
>> capabilities, it could replace some of the AOT-linked classes with
>> alternative versions.
>> - If the VM is started with certain module options, such as
>> `--patch-module` or `--module`, some AOT-linked classes may be replaced with
>> patched versions, or may become invisible and cannot be loaded into the JVM.
>> - When incompatible VM options are detected, the JVM will refuse to load an
>> AOT cache that has AOT-linked classes. See
>> `FileMapInfo::validate_aot_class_linking()`.
>> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires
>> `CDSConfig::is_using_full_module_graph()` to be true. This means that the
>> exa...
>
> src/hotspot/share/cds/archiveUtils.cpp line 390:
>
>> 388: return "boot"; // boot classes in java.base
>> 389: } else {
>> 390: return "boot2"; // boot classes outside of java.base
>
> Suggestion: boot -> boot-base, boot2 -> boot-nonbase ?
I prefer boot/boot2 to make the output easier to read. Anyone debugging this
output will need to read the code to understand what "boot2" or "boot-nonbase"
is. A few extra characters here will not help.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1747824950
Re: RFR: 8329706: Implement -XX:+AOTClassLinking
On Fri, 6 Sep 2024 05:07:09 GMT, David Holmes wrote:
>> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>>
>> **Overview**
>>
>> - A new `-XX:+AOTClassLinking` flag is added. See [JEP
>> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the
>> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this
>> command-line option, its default value, and its impact on compatibility.
>> - When this flag is enabled during the creation of an AOT cache (aka CDS
>> archive), an `AOTLinkedClassTable` is added to the cache to include all
>> classes that are AOT-linked. For this PR, only classes for the
>> boot/platform/application loaders are eligible. The main logic is in
>> `aotClassLinker.cpp`.
>> - When an AOT archive is loaded in a production run, all classes in the
>> `AOTLinkedClassTable` are loaded into their respective class loaders at the
>> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`.
>> - The boot classes are loaded as part of `vmClasses::resolve_all()`
>> - The platform/application classes are loaded after the module graph is
>> restored (see changes in `threads.cpp`).
>> - Since all classes in a `AOTLinkedClassTable` are loaded before any
>> user-code is executed, we can resolve constant pool entries that refer to
>> these classes during AOT cache creation. See changes in
>> `AOTConstantPoolResolver::is_class_resolution_deterministic()`.
>>
>> **All-or-nothing Loading**
>>
>> - Because AOT-linked classes can refer to each other, using direct C++
>> pointers, all AOT-linked classes must be loaded together. Otherwise we will
>> have dangling C++ pointers in the class metadata structures.
>> - During a production run, we check during VM start-up for incompatible VM
>> options that would prevent some of the AOT-linked classes from being loaded.
>> For example:
>> - If the VM is started with an JVMTI agent that has ClassFileLoadHook
>> capabilities, it could replace some of the AOT-linked classes with
>> alternative versions.
>> - If the VM is started with certain module options, such as
>> `--patch-module` or `--module`, some AOT-linked classes may be replaced with
>> patched versions, or may become invisible and cannot be loaded into the JVM.
>> - When incompatible VM options are detected, the JVM will refuse to load an
>> AOT cache that has AOT-linked classes. See
>> `FileMapInfo::validate_aot_class_linking()`.
>> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires
>> `CDSConfig::is_using_full_module_graph()` to be true. This means that the
>> exa...
>
> src/hotspot/share/cds/archiveBuilder.cpp line 904:
>
>> 902: log_info(cds)("instance classes " STATS_FORMAT,
>> STATS_PARAMS(instance_klasses));
>> 903: log_info(cds)(" boot " STATS_FORMAT,
>> STATS_PARAMS(boot_klasses));
>> 904: log_info(cds)(" vm " STATS_FORMAT,
>> STATS_PARAMS(vm_klasses));
>
> Suggestion:
>
> log_info(cds)(" vm " STATS_FORMAT,
> STATS_PARAMS(vm_klasses));
The indentation is intentional: vm is a subset of boot classes, which is a
subset of instance classes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1747823070
RFR: 8329706: Implement -XX:+AOTClassLinking
This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & Linking](https://bugs.openjdk.org/browse/JDK-8315737). **Overview** - A new `-XX:+AOTClassLinking` flag is added. See [JEP 498](https://bugs.openjdk.org/browse/JDK-8315737) and the [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this command-line option, its default value, and its impact on compatibility. - When this flag is enabled during the creation of an AOT cache (aka CDS archive), an `AOTLinkedClassTable` is added to the cache to include all classes that are AOT-linked. For this PR, only classes for the boot/platform/application loaders are eligible. The main logic is in `aotClassLinker.cpp`. - When an AOT archive is loaded in a production run, all classes in the `AOTLinkedClassTable` are loaded into their respective class loaders at the earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. - The boot classes are loaded as part of `vmClasses::resolve_all()` - The platform/application classes are loaded after the module graph is restored (see changes in `threads.cpp`). - Since all classes in a `AOTLinkedClassTable` are loaded before any user-code is executed, we can resolve constant pool entries that refer to these classes during AOT cache creation. See changes in `AOTConstantPoolResolver::is_class_resolution_deterministic()`. **All-or-nothing Loading** - Because AOT-linked classes can refer to each other, using direct C++ pointers, all AOT-linked classes must be loaded together. Otherwise we will have dangling C++ pointers in the class metadata structures. - During a production run, we check during VM start-up for incompatible VM options that would prevent some of the AOT-linked classes from being loaded. For example: - If the VM is started with an JVMTI agent that has ClassFileLoadHook capabilities, it could replace some of the AOT-linked classes with alternative versions. - If the VM is started with certain module options, such as `--patch-module` or `--module`, some AOT-linked classes may be replaced with patched versions, or may become invisible and cannot be loaded into the JVM. - When incompatible VM options are detected, the JVM will refuse to load an AOT cache that has AOT-linked classes. See `FileMapInfo::validate_aot_class_linking()`. - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires `CDSConfig::is_using_full_module_graph()` to be true. This means that the exact same set of modules are visible when the AOT cache was created, and when the AOT cache is used. **Testing** - New test cases are added to test specific behaviors of the `-XX:+AOTClassLinking` flag - A new test group `hotspot_aot_classlinking` is created for running existing CDS tests with the `-XX:+AOTClassLinking` flag. Note that this group filters out test cases that are incompatible with `-XX:+AOTClassLinking`. E.g., classes that use JVMTI ClassFileLoadHook or class redefinition. - We will also modify some of our internal test cases (e.g., with large applications) to also run with the `-XX:+AOTClassLinking` flag. --- See [here](https://bugs.openjdk.org/browse/JDK-8315737) for the sequence of dependent RFEs for implementing JEP 483. - Depends on: https://git.openjdk.org/jdk/pull/20517 Commit messages: - More clean up - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking - 8329706: Implement -XX:+AOTClassLinking Changes: https://git.openjdk.org/jdk/pull/20843/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20843&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329706 Stats: 1734 lines in 46 files changed: 1577 ins; 52 del; 105 mod Patch: https://git.openjdk.org/jdk/pull/20843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20843/head:pull/20843 PR: https://git.openjdk.org/jdk/pull/20843
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v6]
On Thu, 29 Aug 2024 09:28:50 GMT, Stefan Karlsson wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix bit counts in GCForwarding > > src/hotspot/share/cds/archiveHeapWriter.cpp line 214: > >> 212: oopDesc::set_mark(mem, markWord::prototype()); >> 213: oopDesc::release_set_klass(mem, k); >> 214: } > > The `UseCompactObjectHeaders` path calls `get_requested_narrow_klass`, while > the `else` part directly uses `k`. Is one of these paths incorrect? This seems odd. The original code sets `Universe::objectArrayKlass()` into the object header. This is the value of this class in the current JVM lifetime. Later, `ArchiveHeapWriter::update_header_for_requested_obj()` would change the object's klass to the "requested" address. I.e., where this class will be loaded in a future JVM lifetime when the CDS archive is loaded into memory. It seems the same logic should be used in the `UseCompactObjectHeaders==true` case. BTW (unrelated to this PR) the comment a few lines up is outdated and wrong: Klass* k = Universe::objectArrayKlass(); // already relocated to point to archived klass `k` is the value of the *actual* location of this class in the current JVM lifetime. Please ignore this comment when trying to understand this function. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1737294872
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v8]
On Fri, 28 Jun 2024 15:41:48 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 18 commits: > > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Refactor mount info matching to helper function > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Remove problem listing of PlainRead which is reworked here > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - ... and 8 more: https://git.openjdk.org/jdk/compare/486aa11e...1017da35 Looks reasonable to me - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18201#pullrequestreview-2149956104
Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v3]
On Thu, 9 May 2024 18:31:24 GMT, Matias Saavedra Silva
wrote:
>> The beginning of the RW region contains pointers to c++ vtables which are
>> always located at a fixed offset from the shared base address at runtime.
>> This offset can be calculated at dumptime and stored with the read-only
>> tables at the top of the RO region. As a further improvement, all the
>> pointers to RO tables are replaced with offsets as well.
>>
>> These changes will reduce the number of pointers in the RW and RO regions
>> and will allow for the relocation bitmap size optimizations to be more
>> effective. Verified with tier 1-5 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 12 additional
> commits since the last revision:
>
> - Merge branch 'master' into pointer_to_offset_8329418
> - Ioi comments
> - Chris comments and cleanup
> - Merge branch 'master' into pointer_to_offset_8329418
> - Cleanup
> - Corrected SA
> - Editing SA
> - Fixed dynamic dumping
> - Now works with -Xshare:on
> - Adjusted serialization
> - ... and 2 more: https://git.openjdk.org/jdk/compare/c6bab5bc...11f39483
LGTM. Just one small nit.
src/hotspot/share/cds/serializeClosure.hpp line 52:
> 50:
> 51: // Iterate on the pointers from p[0] through p[num_pointers-1]
> 52: void do_ptrs(u_char* start, size_t size) {
I think it will be more consistent if we use the same `void** p` parameter as
in `do_ptr()`. The `u_char*` here is a historical oddity and should be fixed.
-
Marked as reviewed by iklam (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19107#pullrequestreview-2048708441
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1595908706
Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v2]
On Tue, 7 May 2024 16:38:23 GMT, Matias Saavedra Silva
wrote:
>> The beginning of the RW region contains pointers to c++ vtables which are
>> always located at a fixed offset from the shared base address at runtime.
>> This offset can be calculated at dumptime and stored with the read-only
>> tables at the top of the RO region. As a further improvement, all the
>> pointers to RO tables are replaced with offsets as well.
>>
>> These changes will reduce the number of pointers in the RW and RO regions
>> and will allow for the relocation bitmap size optimizations to be more
>> effective. Verified with tier 1-5 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Chris comments and cleanup
Looks good. I will suggest making some clean up to improve the readability of
the existing code.
src/hotspot/share/cds/archiveUtils.cpp line 325:
> 323: assert((intptr_t)obj >= 0 || (intptr_t)obj < -100,
> 324: "hit tag while initializing ptrs.");
> 325: *p = obj != 0 ? (void*)(SharedBaseAddress + obj) : (void*)obj;
`nullptr` should be used instead of `0`. E.g., `obj != (void*)nullptr`
src/hotspot/share/cds/cppVtables.cpp line 236:
> 234: if (!soc->reading()) {
> 235: _vtables_serialized_base = soc->region_top();
> 236: }
The new `region_top()` API may be confusing with the existing `do_region()`
API, which has a completely different meaning for `region`.
I think it's better to rename `do_region()` to
// iterate on the pointers from p[0] through p[num_pointers-1]
SerializeClosure do_ptrs(void** p, int num_pointers);
Also, there's no need to add a new `region_top()` API -- there are already too
many functions that deal with a "region" of different types, and you need to
wonder what this particular "region" is.
`soc->region_top()` can be replaced with
`ArchiveBuilder::current()->current_dump_space()->top()`
Also, in archiveBuilder.hpp, `dump_space` means the same thing as
`dump_region`. All of the former should be changed to the latter for uniformity.
-
Changes requested by iklam (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19107#pullrequestreview-2046836828
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1594750309
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1594766914
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v4]
On Tue, 2 Apr 2024 20:18:34 GMT, Kim Barrett wrote: > > Thanks for reviewing, Kim. Is your suggestion to not have a JVMFlag object > > for develop flags in PRODUCT builds? Presumably to save some footprint? I'm > > not sure we would win fighting the macros to accomplish this. > > Yes, that's the suggestion and the rationale for it. It should also remove > the need for is_constant_in_binary. I don't know how hard it would actually > be to accomplish this. I agree it might not be worth the effort, but we won't > know until someone looks, which I haven't done. It might even be easy. Currently the VM prints an error message for non-product flags, so we need to keep some information about them. We can probably skip the type information, etc, to save a little space, but the space saving would be minimal. $ java -XX:+LoomDeoptAfterThaw --version Error: VM option 'LoomDeoptAfterThaw' is develop and is available only in debug version of VM. Improperly specified VM option 'LoomDeoptAfterThaw' Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. - PR Comment: https://git.openjdk.org/jdk/pull/18541#issuecomment-2033183718
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v2]
On Tue, 2 Apr 2024 16:21:15 GMT, Coleen Phillimore wrote: >> Remove the notproduct distinction for command line options, rather than >> trying to wrestle the macros to fix the bug that they've been treated as >> develop options for some time now. This simplifies the command line option >> macros. >> >> Tested with tier1-4, tier1 on Oracle platforms. Also built shenandoah. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Clean up notproduct from tests. LGTM - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18541#pullrequestreview-1974322553
Re: RFR: 8236736: Change notproduct JVM flags to develop flags
On Thu, 28 Mar 2024 22:53:22 GMT, Coleen Phillimore wrote: > Remove the notproduct distinction for command line options, rather than > trying to wrestle the macros to fix the bug that they've been treated as > develop options for some time now. This simplifies the command line option > macros. > > Tested with tier1-4, tier1 on Oracle platforms. Also built shenandoah. LGTM. For the past 15 years, "notproduct" flags haven't been working as they claim to be in globals.hpp. That doesn't seem to have bothered anyone. This definitely looks like a design that no one needs and should be removed for simplcity. There are some references to "notproduct" in test/hotspot/jtreg/runtime/CommandLine that need to be removed. - Changes requested by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18541#pullrequestreview-1974267126
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Tue, 30 Jan 2024 12:13:00 GMT, Andrew Haley wrote: > > The only "perfect" solution is putting the HotSpot code in a namespace. > > This is going to be a huge undertaking. I don't think we have enough > > interest in the OpenJDK community to make such a change now. > > I don't think that putting all of the HotSpot code in a namespace. At least, > I hope not: it'll mess up debugging so much that it'll be intolerable, IMO, > and there will be other side effects. I forgot to qualify "perfect" only in the sense of isolating the HotSpot symbols. It's obviously not perfect at all in other aspects. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1916772980
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Tue, 30 Jan 2024 10:27:08 GMT, Andrew Haley wrote: > > > > Maybe we could live with symbol redefinition using #define > > > > (conditionally for static linking in OpenJDK, as Coleen suggested > > > > earlier) for now, until the tooling can support symbol localizing > > > > better. Then localizing symbols using tools like `objcopy` can be the > > > > longer term and cleaner solution, instead of using namespace. What's > > > > your thoughts on that? > > > > > > > > > I suppose so, but why? > > > Why should any of this have to work on old systems? If their binutils is > > > broken, static linking of openjdk won't work there. > > > > > > We ran into issues with older gcc on linux-aarch for partial linking, but > > the problem may not be older gcc only(?). At the current stage, limiting > > static/hermetic Java runtime support to only the platforms that support > > partial linking and `objcopy` seems to be overly restrictive (it does > > simplify the requirements significantly however :-)): > > The duplicate symbol problems are mostly found in JDK natives and have been > > resolved already. We've found very few symbol issues with hotspot code so > > far. As there are portable alternative solutions that can resolve the > > symbol issues in hotspot, choosing a less portable solution seems not too > > attractive currently. > > I believe this to be a mistake. HotSpot, by design, exports only the symbols > intended for use by other components. Many of the symbol names are highly > generic, and will conflict with application code. > > Sure, you have enough to be able to do some prototyping, but for real-world > deployment you must be able to control symbol exports. I agree with Andrew. We don't want the perfect to be the enemy of the good. The only "perfect" solution is putting the HotSpot code in a namespace. This is going to be a huge undertaking. I don't think we have enough interest in the OpenJDK community to make such a change now. I think partial linking with objcopy is a clean solution that's good enough for the actual use cases. If someone wants to use `#define`, they can just make a local branch and add a few `#define` lines in their globalDefinitions.hpp. I suspect the configure script also allows adding C compiler options like `-DThread=HSThread`. `#define` is going to be a whack-a-mole hack. Google may need to isolate the `Thread` symbol, but other people may need to isolate things like `Symbol`, etc. It's not a good idea to add arbitrary `#define` in the HotSpot source code just because someone doesn't like it. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1916692309
Re: RFR: 8323546: Cleanup jcmd docs for Compiler.perfmap and VM.cds filename parameter [v6]
On Wed, 17 Jan 2024 02:26:22 GMT, Chris Plummer wrote: >> The jcmd docs for Compiler.perfmap currently say: >> >> - *filename*: (Optional) The name of the map file (STRING, no default >> value) >> >> However, there is a default, so not only should that be made more clear in >> the above, but also some descriptive text as to how the default is generated >> should be added. >> >> VM.cds has a similar issue, but already has the descriptive text, so just >> the "no default value" part needs to be fixed. >> >> Another change needed is to consistently use *filename* (italics) instead of >> `filename` (monospace). Note this is how html formatting is done. For the >> man page formatting, *filename* does no formatting and `filename` is >> displayed in color if supported. Personally I prefer `filename`, but it >> seems that there is already a strong precedence for using italics in the >> *arguments* list. For example: >> >> *arguments*: >> >> - *flag name*: The name of the flag that you want to set (STRING, no >> default value) >> >> - *string value*: (Optional) The value that you want to set (STRING, no >> default value) > > Chris Plummer has updated the pull request incrementally with two additional > commits since the last revision: > > - Get rid of extra empty line. > - Remove quotes from around filename to be consistent with how jcmd STRING > defaults are printed. LGTM. I think having one test case for perfmap should be enough. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17359#pullrequestreview-1826632142
Re: RFR: 8323546: Clarify docs for Compiler.perfmap filename parameter, and other misc related jcmd doc cleanups [v4]
On Mon, 15 Jan 2024 07:32:31 GMT, David Holmes wrote:
>> This check is problematic:
>>
>>
>> if (strncmp(filename, DEFAULT_PERFMAP_FILENAME,
>> strlen(DEFAULT_PERFMAP_FILENAME)) == 0)
>>
>>
>> Because it cannot tell whether `filename` was explicitly given by the user.
>> As a result, if the user specifies:
>>
>>
>> jcmd 1234 perfmap '/tmp/perf-.map'
>>
>>
>> the file will be written as `/tmp/perf-1234.map`, but if the user specifies
>>
>>
>> jcmd 1234 perfmap '/tmp/-perf.map'
>>
>>
>> then the file will be written as `/tmp/-perf.map`.
>>
>> This gives the confusing impression that the string `` is sometimes
>> substituted and sometimes not.
>>
>> I think it's better to do this (similarly for the `VM.cds` case):
>>
>>
>> void PerfMapDCmd::execute(DCmdSource source, TRAPS) {
>> CodeCache::write_perf_map(_filename.is_set() ? _filename.value() :
>> nullptr);
>> }
>>
>>
>> This essentially makes the JVM behavior exactly the same as before, so I
>> think we can change the JBS issue title to something like "Clarify docs for
>> filename parameter to Compiler.perfmap and VM.cds".
>
> @iklam but IIUC that does not address the issue of updationg the help output.
My suggestion is to be used in combination with this change:
_filename("filename", "Name of the map file", "STRING", false,
DEFAULT_PERFMAP_FILENAME)
So the help message will be printed as:
*filename*: (Optional) The name of the map file (STRING /tmp/perf-.map)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17359#discussion_r1452040385
Re: RFR: 8323546: Clarify docs for Compiler.perfmap filename parameter, and other misc related jcmd doc cleanups [v4]
On Mon, 15 Jan 2024 01:29:47 GMT, David Holmes wrote:
>> The default value for the argument is what gets displayed in the help text.
>> For example:
>>
>>
>> ThreadDumpToFileDCmd::ThreadDumpToFileDCmd(outputStream* output, bool heap) :
>>DCmdWithParser(output, heap),
>> _overwrite("-overwrite", "May overwrite existing file", "BOOLEAN", false,
>> "false"),
>> _format("-format", "Output format ("plain" or "json")", "STRING", false,
>> "plain"),
>> _filepath("filepath", "The file path to the output file", "STRING", true) {
>> _dcmdparser.add_dcmd_option(&_overwrite);
>> _dcmdparser.add_dcmd_option(&_format);
>> _dcmdparser.add_dcmd_argument(&_filepath);
>> }
>>
>>
>> And the help text:
>>
>>
>> Options: (options must be specified using the or = syntax)
>> -overwrite : [optional] May overwrite existing file (BOOLEAN, false)
>> -format : [optional] Output format ("plain" or "json") (STRING, plain)
>>
>>
>> The help output that indicates that "plain" is the default format comes from
>> the intialization of the _format argument. There is no separate help text.
>
> Ugghh! So the help text is an actual stringification of the actual default
> value of the "field", whereas in this case the real default value comes from
> passing null to `CodeCache::write_perf_map`. So we need this hack to deal
> with that. That is truly awful IMO. The only way to cleanly address that is
> to expand things so that you can set an actual help string to be used.
This check is problematic:
if (strncmp(filename, DEFAULT_PERFMAP_FILENAME,
strlen(DEFAULT_PERFMAP_FILENAME)) == 0)
Because it cannot tell whether `filename` was explicitly given by the user. As
a result, if the user specifies:
jcmd 1234 perfmap '/tmp/perf-.map'
the file will be written as `/tmp/perf-1234.map`, but if the user specifies
jcmd 1234 perfmap '/tmp/-perf.map'
then the file will be written as `/tmp/-perf.map`.
This gives the confusing impression that the string `` is sometimes
substituted and sometimes not.
I think it's better to do this (similarly for the `VM.cds` case):
void PerfMapDCmd::execute(DCmdSource source, TRAPS) {
CodeCache::write_perf_map(_filename.is_set() ? _filename.value() : nullptr);
}
This essentially makes the JVM behavior exactly the same as before, so I think
we can change the JBS issue title to something like "Clarify docs for filename
parameter to Compiler.perfmap and VM.cds".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17359#discussion_r1451918737
Integrated: 8320935: Move CDS config initialization code to cdsConfig.cpp
On Tue, 28 Nov 2023 23:24:53 GMT, Ioi Lam wrote: > This is a simple clean up that moves the code for initializing the CDS config > states from arguments.cpp to cdsConfig.cpp > > I renamed a few functions, but otherwise the code is unchanged. > > - `get_default_shared_archive_path()` -> `default_archive_path()` > - `GetSharedArchivePath()` -> `static_archive_path()` > - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` > > There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is > compiled only if CDS is enabled. This pull request has now been integrated. Changeset: 4c96aac9 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/4c96aac9c0aa450b0b6859ded8dfff856222ad58 Stats: 696 lines in 8 files changed: 346 ins; 327 del; 23 mod 8320935: Move CDS config initialization code to cdsConfig.cpp Reviewed-by: ccheung, matsaave, stuefe - PR: https://git.openjdk.org/jdk/pull/16868
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v3]
On Sat, 2 Dec 2023 03:36:05 GMT, Calvin Cheung 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 seven additional commits >> since the last revision: >> >> - Merge branch 'master' into 8320935-move-cds-config-code-from-arguments-cpp >> - fixed indentation >> - code alignment >> - step4 >> - step3 >> - step2 >> - step1 > > Marked as reviewed by ccheung (Reviewer). Thanks @calvinccheung @matias9927 @tstuefe for the review. - PR Comment: https://git.openjdk.org/jdk/pull/16868#issuecomment-1842107804
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v3]
> This is a simple clean up that moves the code for initializing the CDS config > states from arguments.cpp to cdsConfig.cpp > > I renamed a few functions, but otherwise the code is unchanged. > > - `get_default_shared_archive_path()` -> `default_archive_path()` > - `GetSharedArchivePath()` -> `static_archive_path()` > - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` > > There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is > compiled only if CDS is enabled. 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: - Merge branch 'master' into 8320935-move-cds-config-code-from-arguments-cpp - fixed indentation - code alignment - step4 - step3 - step2 - step1 - Changes: - all: https://git.openjdk.org/jdk/pull/16868/files - new: https://git.openjdk.org/jdk/pull/16868/files/01dd47bc..a080edeb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16868&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16868&range=01-02 Stats: 84382 lines in 1756 files changed: 39063 ins; 38780 del; 6539 mod Patch: https://git.openjdk.org/jdk/pull/16868.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16868/head:pull/16868 PR: https://git.openjdk.org/jdk/pull/16868
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Wed, 29 Nov 2023 21:53:06 GMT, Calvin Cheung wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fixed indentation > > src/hotspot/share/cds/cdsConfig.cpp line 34: > >> 32: #include "logging/log.hpp" >> 33: #include "runtime/arguments.hpp" >> 34: #include "runtime/java.hpp" > > I was able to build with your patch without including `java.hpp`. > The #include java.hpp could also be removed from arguments.cpp. cdsConfig.cpp needs the declaration of `vm_exit_during_initialization()` from java.hpp. Although java.hpp is included by arguments.hpp, we usually try to avoid such indirectly inclusions. Otherwise if arguments.hpp is changed to no longer include java.hpp, then cdsConfig.hpp will fail to compile. I am not sure about arguments.cpp -- if java.hpp is already included by arguments.hpp, do we need to explicitly include it in arguments.cpp? I'll leave that alone in this PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412682898
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Fri, 1 Dec 2023 22:04:22 GMT, Matias Saavedra Silva
wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed indentation
>
> src/hotspot/share/cds/cdsConfig.cpp line 101:
>
>> 99: void CDSConfig::extract_shared_archive_paths(const char* archive_path,
>> 100: char** base_archive_path,
>> 101: char** top_archive_path) {
>
> Could you align these arguments?
Fixed.
> src/hotspot/share/cds/cdsConfig.cpp line 125:
>
>> 123: }
>> 124:
>> 125: void CDSConfig::init_shared_archive_paths() {
>
> Now that I see this there is a lot of indentation thanks to the nested
> conditionals. I don't have much to offer but is there a cleaner way to format
> this method? Maybe you can extract the code in `if (archives == 1)` into its
> own method for better readability.
I want to keep the code change minimal while moving code from one file to
another. I'll refactor this function in a follow-on PR. That way it will be
easier to track the code history.
> src/hotspot/share/runtime/arguments.cpp line 1262:
>
>> 1260: }
>> 1261:
>> 1262: CDSConfig::check_system_property(key, value);
>
> I see this is only called once, do you expect this method to be used again?
> It may be unnecessary to extract this code into its own method.
I wanted to move the code from arguments.cpp to cdsConfig.cpp, so I had to put
it in a new function.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683767
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683760
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683786
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
> This is a simple clean up that moves the code for initializing the CDS config > states from arguments.cpp to cdsConfig.cpp > > I renamed a few functions, but otherwise the code is unchanged. > > - `get_default_shared_archive_path()` -> `default_archive_path()` > - `GetSharedArchivePath()` -> `static_archive_path()` > - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` > > There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is > compiled only if CDS is enabled. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: fixed indentation - Changes: - all: https://git.openjdk.org/jdk/pull/16868/files - new: https://git.openjdk.org/jdk/pull/16868/files/72f3e44c..01dd47bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16868&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16868&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16868.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16868/head:pull/16868 PR: https://git.openjdk.org/jdk/pull/16868
RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp
This is a simple clean up that moves the code for initializing the CDS config states from arguments.cpp to cdsConfig.cpp I renamed a few functions, but otherwise the code is unchanged. - `get_default_shared_archive_path()` -> `default_archive_path()` - `GetSharedArchivePath()` -> `static_archive_path()` - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is compiled only if CDS is enabled. - Commit messages: - code alignment - step4 - step3 - step2 - step1 Changes: https://git.openjdk.org/jdk/pull/16868/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16868&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320935 Stats: 696 lines in 8 files changed: 346 ins; 327 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/16868.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16868/head:pull/16868 PR: https://git.openjdk.org/jdk/pull/16868
Re: RFR: 8318484: Initial version of cdsConfig.hpp [v2]
On Thu, 19 Oct 2023 06:58:22 GMT, David Holmes 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 8318484-initial-version-of-cdsConfig-hpp >> - 8318484: Initial version of cdsConfig.hpp > > Initial refactoring looks good. One query below. > > Thanks Thanks @dholmes-ora @calvinccheung @sspitsyn for the review. - PR Comment: https://git.openjdk.org/jdk/pull/16257#issuecomment-1773834764
Integrated: 8318484: Initial version of cdsConfig.hpp
On Thu, 19 Oct 2023 05:56:53 GMT, Ioi Lam wrote:
> This is the first step for [JDK-8318483 - Move CDS configuration management
> into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
>
> - Remove `Arguments::is_dumping_archive()` and `Arguments
> assert_is_dumping_archive()`
> - Add the following new APIs
>
>
> class CDSConfig {
> static bool is_dumping_archive();
> static bool is_dumping_static_archive();
> static bool is_dumping_dynamic_archive();
> static bool is_dumping_heap();
> };
>
>
> - Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to
> these new APIs
>
> (More APIs will be added in future sub tasks of
> [JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
This pull request has now been integrated.
Changeset: ecd25e7d
Author:Ioi Lam
URL:
https://git.openjdk.org/jdk/commit/ecd25e7d6f9d69f9dbdbff0a4a9b9d6b19288593
Stats: 236 lines in 36 files changed: 125 ins; 16 del; 95 mod
8318484: Initial version of cdsConfig.hpp
Reviewed-by: dholmes, ccheung, sspitsyn
-
PR: https://git.openjdk.org/jdk/pull/16257
Re: RFR: 8318484: Initial version of cdsConfig.hpp [v2]
> This is the first step for [JDK-8318483 - Move CDS configuration management
> into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
>
> - Remove `Arguments::is_dumping_archive()` and `Arguments
> assert_is_dumping_archive()`
> - Add the following new APIs
>
>
> class CDSConfig {
> static bool is_dumping_archive();
> static bool is_dumping_static_archive();
> static bool is_dumping_dynamic_archive();
> static bool is_dumping_heap();
> };
>
>
> - Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to
> these new APIs
>
> (More APIs will be added in future sub tasks of
> [JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
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 8318484-initial-version-of-cdsConfig-hpp
- 8318484: Initial version of cdsConfig.hpp
-
Changes:
- all: https://git.openjdk.org/jdk/pull/16257/files
- new: https://git.openjdk.org/jdk/pull/16257/files/0a2f78b0..0d729778
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=16257&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=16257&range=00-01
Stats: 5386 lines in 163 files changed: 3808 ins; 820 del; 758 mod
Patch: https://git.openjdk.org/jdk/pull/16257.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16257/head:pull/16257
PR: https://git.openjdk.org/jdk/pull/16257
Re: RFR: 8318484: Initial version of cdsConfig.hpp
On Thu, 19 Oct 2023 22:21:30 GMT, Calvin Cheung wrote:
>> This is the first step for [JDK-8318483 - Move CDS configuration management
>> into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
>>
>> - Remove `Arguments::is_dumping_archive()` and `Arguments
>> assert_is_dumping_archive()`
>> - Add the following new APIs
>>
>>
>> class CDSConfig {
>> static bool is_dumping_archive();
>> static bool is_dumping_static_archive();
>> static bool is_dumping_dynamic_archive();
>> static bool is_dumping_heap();
>> };
>>
>>
>> - Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to
>> these new APIs
>>
>> (More APIs will be added in future sub tasks of
>> [JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
>
> src/hotspot/share/cds/cdsConfig.hpp line 39:
>
>> 37:
>> 38: // CDS archived heap
>> 39: static bool is_dumping_heap()
>> NOT_CDS_JAVA_HEAP_RETURN_(false);
>
> Too much blank spaces between the function declarations and NOT_CDS_* macros.
> The function declarations could also be shifted more to the left.
The spaces are for functions that will be added in the next PR that have longer
names:
// Basic CDS features
static bool is_dumping_archive()
NOT_CDS_RETURN_(false);
static bool is_dumping_static_archive()
NOT_CDS_RETURN_(false);
static bool is_dumping_dynamic_archive()
NOT_CDS_RETURN_(false);
// CDS archived heap
static bool is_dumping_heap()
NOT_CDS_JAVA_HEAP_RETURN_(false);
static bool is_loading_heap()
NOT_CDS_JAVA_HEAP_RETURN_(false);
static void disable_dumping_full_module_graph(const char* reason = nullptr)
NOT_CDS_JAVA_HEAP_RETURN;
static bool is_dumping_full_module_graph()
NOT_CDS_JAVA_HEAP_RETURN_(false);
static void disable_loading_full_module_graph(const char* reason = nullptr)
NOT_CDS_JAVA_HEAP_RETURN;
static bool is_loading_full_module_graph()
NOT_CDS_JAVA_HEAP_RETURN_(false);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16257#discussion_r1366367480
Re: RFR: 8318484: Initial version of cdsConfig.hpp
On Thu, 19 Oct 2023 06:54:05 GMT, David Holmes wrote:
>> This is the first step for [JDK-8318483 - Move CDS configuration management
>> into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
>>
>> - Remove `Arguments::is_dumping_archive()` and `Arguments
>> assert_is_dumping_archive()`
>> - Add the following new APIs
>>
>>
>> class CDSConfig {
>> static bool is_dumping_archive();
>> static bool is_dumping_static_archive();
>> static bool is_dumping_dynamic_archive();
>> static bool is_dumping_heap();
>> };
>>
>>
>> - Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to
>> these new APIs
>>
>> (More APIs will be added in future sub tasks of
>> [JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
>
> src/hotspot/share/cds/metaspaceShared.cpp line 778:
>
>> 776:
>> 777: #if INCLUDE_CDS_JAVA_HEAP
>> 778: if (CDSConfig::is_dumping_heap()) {
>
> This seems a new condition. Why is it needed now?
This was a bug uncovered during refactoring.
`StringTable::allocate_shared_strings_array()` used to assert
`DumpSharedSpaces`. However, this function is useful only in a more limited
scope (`CDSConfig::is_dumping_heap()` which is a subset of `DumpSharedSpaces`).
So after changing the assert in
`StringTable::allocate_shared_strings_array()`, I have to change the condition
where this function is called.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16257#discussion_r1366041643
RFR: 8318484: Initial version of cdsConfig.hpp
This is the first step for [JDK-8318483 - Move CDS configuration management
into cdsConfig.hpp](https://bugs.openjdk.org/browse/JDK-8318483)
- Remove `Arguments::is_dumping_archive()` and `Arguments
assert_is_dumping_archive()`
- Add the following new APIs
class CDSConfig {
static bool is_dumping_archive();
static bool is_dumping_static_archive();
static bool is_dumping_dynamic_archive();
static bool is_dumping_heap();
};
- Convert some use of `DumpSharedSpaces` and `DynamicDumpSharedSpaces` to these
new APIs
(More APIs will be added in future sub tasks of
[JDK-8318483](https://bugs.openjdk.org/browse/JDK-8318483))
-
Commit messages:
- 8318484: Initial version of cdsConfig.hpp
Changes: https://git.openjdk.org/jdk/pull/16257/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16257&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8318484
Stats: 236 lines in 36 files changed: 125 ins; 16 del; 95 mod
Patch: https://git.openjdk.org/jdk/pull/16257.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16257/head:pull/16257
PR: https://git.openjdk.org/jdk/pull/16257
Re: RFR: 8314550: [macosx-aarch64] serviceability/sa/TestJmapCore.java fails with "sun.jvm.hotspot.debugger.UnmappedAddressException: 801000800"
On Thu, 24 Aug 2023 23:31:56 GMT, Chris Plummer wrote: > On some macosx-aarch64 systems, not all mapped pages are dumped to the core > file. This first turned up with > [JDK-8293563](https://bugs.openjdk.org/browse/JDK-8293563) where large parts > of the ZGC heap would not be in the core file, leading to various SA address > errors. For JDK-8293563 the issue was addressed by having the core file test > always use `-XX:+AlwaysPreTouch` on macosx-aarch64. This seemed to force the > ZGC pages to always end up in the core file. > > A similar issue has been noticed with mapped in pages of the CDS archive. We > are seeing cases where SA references to addresses that are clearly in the CDS > archive (based on info in the hs_err file) are failing to be read from the > core file by SA. This problem has turned up a number of times during CI > testing, but I have yet to be able to reproduce it myself. This PR is an > attempt to address this testing issue by having the CDS archive also pretouch > all mapped in pages when `-XX:+AlwaysPreTouch` is used. > > Tested with tier1 and tier3 and also ran the test about 5,000 times with and > without the fix. It never reproduced for either. Hopefully the problem is > gone with this fix, but it may take a few months of CI testing before we can > be confident it is fixed. Looks good to me. The size of the CDS archive is usually much smaller than the committed heap size. The [GC tuning docs says](https://docs.oracle.com/en/java/javase/11/gctuning/garbage-first-garbage-collector-tuning.html#GUID-0770AB01-E334-4E23-B307-FD2114B16E0E): > You can use -XX:+AlwaysPreTouch to move the operating system work to back > virtual memory with physical memory to VM startup time. Both of these > measures can be particularly desirable in order to make pause-times more > consistent. So committing all the CDS regions to physical memory at VM start-up would be what the user expects when `-XX:+AlwaysPreTouch` is specified. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15423#pullrequestreview-1594875750
Re: RFR: JDK-8311870: Split CompressedKlassPointers from compressedOops.hpp
On Tue, 11 Jul 2023 11:20:19 GMT, Thomas Stuefe wrote: > In preparation for some Lilliput-related changes, I'd like to get some purely > mechanical code moves out of the way. It would also improve separation of > concerns and reduces include header bloat. > > In particular, this patch does: > > 1) Move `CompressedKlassPointers` from `compressedOops.(cpp|hpp|inline.hpp)` > to `compressedKlass.(cpp|hpp|inline.hpp)` > > 2) flatten the `NarrowPtrStruct _narrow_klass` to `address _base; int _shift` > (its implicit null check member is not needed for Klass and it has little > merit otherwise). > > 3) moved `narrowKlass` from `oopsHierarchy.hpp` to `compressedKlass.hpp` > > 4) remove `KlassAlignment` and `LogKlassAlignment` (the word-sized variants, > not xxxInBytes) since they are unused > > 5) Move `KlassEncodingMetaspaceMax`, `LogKlassAlignmentInBytes` and > `KlassAlignmentInBytes` to compressedKlass.hpp > > 6) Fixed all include issues (including existing missing includes) > > 7) Fixed VM struct because of (2) > > Note that nothing functional is changed. Looks reasonable to me. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14826#pullrequestreview-1525290781
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 13:17:21 GMT, Coleen Phillimore wrote:
>> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but
>> I've added a pointer delta function in globalDefinitions.hpp to use for
>> these pointer diff calculations that return int everywhere. If the name is
>> agreeable, I'll fix the other cases of this like this. It's better than raw
>> casts.
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use pointer_delta_as_int for the name that uses pointer_delta, fix negative
> case to just do checked_cast.
LGTM.
src/hotspot/share/utilities/globalDefinitions.hpp line 529:
> 527: template
> 528: inline int pointer_delta_as_int(const volatile T* left, const volatile
> T* right) {
> 529: return checked_cast(pointer_delta(left, right, sizeof(T)));
For clarity, I think you should add a comment saying the returned value is
always non-negative.
-
Marked as reviewed by iklam (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14675#pullrequestreview-1503498625
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1245463745
Integrated: 8309878: Reduce inclusion of resolvedIndyEntry.hpp
On Mon, 12 Jun 2023 19:52:36 GMT, Ioi Lam wrote: > resolvedIndyEntry.hpp was added in > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995) and is included in > the popular cpCache.hpp. As a result, resolvedIndyEntry.hpp is included in > 807 out of about 1160 hotspot .o files. > > The contents of resolvedIndyEntry.hpp is infrequently used. Its inclusion > should be moved from cpCache.hpp to cpCache.inline.hpp. This improves hotspot > build time. > > After this PR, resolvedIndyEntry.hpp is included in only 30 hotspot .o files. This pull request has now been integrated. Changeset: 5d193193 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/5d193193a3a4c519e7b3d77b27e6b2bf1b11c7f9 Stats: 86 lines in 35 files changed: 61 ins; 13 del; 12 mod 8309878: Reduce inclusion of resolvedIndyEntry.hpp Reviewed-by: coleenp, sspitsyn, matsaave - PR: https://git.openjdk.org/jdk/pull/14427
Re: RFR: 8309878: Reduce inclusion of resolvedIndyEntry.hpp
On Mon, 12 Jun 2023 22:24:26 GMT, Serguei Spitsyn wrote: >> resolvedIndyEntry.hpp was added in >> [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995) and is included >> in the popular cpCache.hpp. As a result, resolvedIndyEntry.hpp is included >> in 807 out of about 1160 hotspot .o files. >> >> The contents of resolvedIndyEntry.hpp is infrequently used. Its inclusion >> should be moved from cpCache.hpp to cpCache.inline.hpp. This improves >> hotspot build time. >> >> After this PR, resolvedIndyEntry.hpp is included in only 30 hotspot .o files. > > Looks good. > Thanks, > Serguei Thanks @sspitsyn @coleenp @matias9927 for the review. - PR Comment: https://git.openjdk.org/jdk/pull/14427#issuecomment-1590238854
RFR: 8309878: Reduce inclusion of resolvedIndyEntry.hpp
resolvedIndyEntry.hpp was added in [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995) and is included in the popular cpCache.hpp. As a result, resolvedIndyEntry.hpp is included in 807 out of about 1160 hotspot .o files. The contents of resolvedIndyEntry.hpp is infrequently used. Its inclusion should be moved from cpCache.hpp to cpCache.inline.hpp. This improves hotspot build time. After this PR, resolvedIndyEntry.hpp is included in only 30 hotspot .o files. - Commit messages: - fixed copyright - step2 - step1 Changes: https://git.openjdk.org/jdk/pull/14427/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14427&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309878 Stats: 86 lines in 35 files changed: 61 ins; 13 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/14427.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14427/head:pull/14427 PR: https://git.openjdk.org/jdk/pull/14427
Re: RFR: 8309673: Refactor ref_at methods in SA ConstantPool [v2]
On Fri, 9 Jun 2023 18:14:18 GMT, Matias Saavedra Silva wrote: >> The accessor methods in constantpool.cpp were previously cleaned up to allow >> for different types of indices to be used, distinguishing them by the >> bytecode. This patch adds the same changes to the hotspot serviceability >> agent code. Verified with tier 1-5 tests. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Removed unnecessary assignment LGTM. I would suggest changing "Serviceability" in the title to "SA" - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14385#pullrequestreview-1472921430
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v4]
On Thu, 12 Jan 2023 16:52:50 GMT, Thomas Stuefe wrote: >> Curious, I always thought we do ArrayAllocator - using mmap for larger >> allocations - to prevent memory retention for libc variants whose allocators >> are "grabby", i.e. which don't promptly return memory to the OS on free(). >> E.g. because they only use sbrk (Solaris, AIX), or are just cautious about >> returning memory (glibc). >> >> Glibc's retention problem is only relevant for fine-grained allocations, so >> for glibc this is probably fine. This leaves at least AIX as a potential >> problem. @backwaterred, does the AIX libc malloc() still exclusively use the >> data segment ? Does free'd memory still stick to the process? >> >> (While writing this, I remember that we at SAP even rewrote Arena allocation >> to use mmap for AIX, because large compile arenas caused lasting RSS >> increase, so it has definitely been a problem in the past) > >> > To follow up on @tstuefe comment - and the one that I tried to say in the >> > bug was that we added this MmapArrayAllocate feature for some G1 marking >> > bits that used so much memory that hit the Solaris _sbrk issue. Maybe >> > @stefank and @tschatzl remember this issue. Maybe it's ok for AIX, then >> > removing this code is a good change. Maybe the G1 usages need a mmap >> > implementation though. >> >> The padding.inline.hpp usage seems to have one caller which is called once. >> The other mmap usage in G1 we can convert to mmap using a similar approach >> to zGranuleMap if that is preferred. That would then be equivalent behavior, >> it looks like the G1 code uses the page allocation granularity anyway so >> maybe keeping it mmap is the better way to go here anyway? > > My uninformed opinion (I'm not the G1 code owner) is that it would be fine to > use explicit mmap. I'd love the complexity reduction this patch brings. @tstuefe @backwaterred I'd like to see this RFE revived. Do we know if anyone is using the `ArrayAllocatorMallocLimit` flag in any production environment today? It seems unlikely to me, as you'd need to explicitly specify `-XX:+UnlockExperimentalVMOptions` in the command-line. And, if this option had been useful (for the AIX port, for example), it would have been changed to a non-experimental (with proper `_pd` support) option over the past 10 years. - PR Comment: https://git.openjdk.org/jdk/pull/11931#issuecomment-1557558015
Re: RFR: 8306843: JVMTI tag map extremely slow after JDK-8292741 [v4]
On Tue, 9 May 2023 14:02:26 GMT, Coleen Phillimore wrote: >> The ResourceHashtable conversion for JDK-8292741 didn't add the resizing >> code. The old hashtable code was tuned for resizing in anticipation of >> large hashtables for JVMTI tags. This patch ports over the old hashtable >> resizing code. It also adds a ResourceHashtable::put_fast() function that >> prepends to the bucket list, which is also reclaims the performance of the >> old hashtable for this test with 10M tags. The ResourceHashtable put >> function is really a put_if_absent. This can be cleaned up in a future >> change. Also, the remove function needed a lambda to destroy the >> WeakHandle, since resizing requires copying entries. >> >> Tested with JVMTI and JDI tests locally, and tier1-4 tests. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > One line and comment making obj null in copy constructor. LGTM - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13818#pullrequestreview-1419201972
Re: RFR: 8306843: JVMTI tag map extremely slow after JDK-8292741 [v2]
On Mon, 8 May 2023 13:59:06 GMT, Coleen Phillimore wrote: >> I would suggest `put_when_absent` to complement `put_if_absent` - with >> suitable descriptive comments of course. > > This is a good name. Updated. I cannot tell the difference between `put_when_absent` and `put_if_absent`. Grammatically they mean the same thing to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187952526
Re: RFR: 8306843: JVMTI tag map extremely slow after JDK-8292741 [v2]
On Fri, 5 May 2023 12:07:20 GMT, Coleen Phillimore wrote:
>> The ResourceHashtable conversion for JDK-8292741 didn't add the resizing
>> code. The old hashtable code was tuned for resizing in anticipation of
>> large hashtables for JVMTI tags. This patch ports over the old hashtable
>> resizing code. It also adds a ResourceHashtable::put_fast() function that
>> prepends to the bucket list, which is also reclaims the performance of the
>> old hashtable for this test with 10M tags. The ResourceHashtable put
>> function is really a put_if_absent. This can be cleaned up in a future
>> change. Also, the remove function needed a lambda to destroy the
>> WeakHandle, since resizing requires copying entries.
>>
>> Tested with JVMTI and JDI tests locally, and tier1-4 tests.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove return variable from remove lambda, fix formatting.
I can't comment on the JVMTI changes, but the changes in the hashtable code
seems OK to me.
src/hotspot/share/classfile/stringTable.cpp line 638:
> 636: public:
> 637: size_t _errors;
> 638: VerifyCompStrings() : _table(unsigned(_items_count / 8) + 1, 0 /* do
> not resize */), _errors(0) {}
Shouldn't this use a regular ResourceHashtable instead?
src/hotspot/share/utilities/resizeableResourceHash.hpp line 91:
> 89: // Calculate next "good" hashtable size based on requested count
> 90: int calculate_resize(bool use_large_table_sizes) const {
> 91: const int resize_factor = 2; // by how much we will resize using
> current number of entries
Does this function depend on the template parameters? If not, I think it can be
made a static function -- you may need to pass `BASE::number_of_entries()` in
as a parameter.
src/hotspot/share/utilities/resourceHash.hpp line 147:
> 145: */
> 146: bool put_fast(K const& key, V const& value) {
> 147: unsigned hv = HASH(key);
I think `put_fast` is not clear enough. Maybe `put_must_be_absent()` or
something more concise.
-
PR Review: https://git.openjdk.org/jdk/pull/13818#pullrequestreview-1416091781
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187009635
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187005281
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187009805
Integrated: 8305771: SA ClassWriter.java fails to skip overpass methods
On Tue, 25 Apr 2023 23:49:56 GMT, Ioi Lam wrote: > Please review this trivial fix. > > When checking for bits in `m.getAccessFlags()`, the mask > `JVM_RECOGNIZED_METHOD_MODIFIERS` should be applied (similar to other uses of > `m.getAccessFlags()` in ClassWriter.java This pull request has now been integrated. Changeset: 750bece0 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/750bece0c2f331025590e7358c7b69f4811f0d24 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8305771: SA ClassWriter.java fails to skip overpass methods Reviewed-by: kevinw, cjplummer - PR: https://git.openjdk.org/jdk/pull/13663
Re: RFR: 8305771: SA ClassWriter.java fails to skip overpass methods
On Wed, 26 Apr 2023 17:43:26 GMT, Kevin Walls wrote: >> Please review this trivial fix. >> >> When checking for bits in `m.getAccessFlags()`, the mask >> `JVM_RECOGNIZED_METHOD_MODIFIERS` should be applied (similar to other uses >> of `m.getAccessFlags()` in ClassWriter.java > > Marked as reviewed by kevinw (Committer). Thanks @kevinjwalls and @plummercj for the review. - PR Comment: https://git.openjdk.org/jdk/pull/13663#issuecomment-1524031268
RFR: 8305771: SA ClassWriter.java fails to skip overpass methods
Please review this trivial fix. When checking for bits in `m.getAccessFlags()`, the mask `JVM_RECOGNIZED_METHOD_MODIFIERS` should be applied (similar to other uses of `m.getAccessFlags()` in ClassWriter.java - Commit messages: - 8305771: SA ClassWriter.java fails to skip overpass methods Changes: https://git.openjdk.org/jdk/pull/13663/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13663&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8305771 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13663.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13663/head:pull/13663 PR: https://git.openjdk.org/jdk/pull/13663
Re: RFR: 8298048: Combine CDS archive heap into a single block
On Tue, 11 Apr 2023 21:47:40 GMT, Ashutosh Mehra wrote: >> This PR combines the "open" and "closed" regions of the CDS archive heap >> into a single region. This significantly simplifies the implementation, >> making it more compatible with non-G1 collectors. There's a net removal of >> ~1100 lines in src code and another ~1200 lines of tests. >> >> **Notes for reviewers:** >> - Most of the code changes in CDS are removing the handling of "open" vs >> "closed" objects. >> - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). >> - It might be easier to see the diff with whitespaces off. >> - There are two major changes in the G1 code >> - The archived objects are now stored in the "old" region (see >> g1CollectedHeap.cpp in >> [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) >> - The majority of the other changes are removal of the "archive" region >> type (see heapRegionType.hpp). For ease of review, such code is isolated in >> [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) >> - Testing changes: >> - Now the archived java objects can move, along with the "old" regions >> that contain them. It's no longer possible to test whether a heap object >> came from CDS. As a result, the `WhiteBox.isShared(Object o)` API has been >> removed. >> - Many tests that uses this API are removed. Most of them were written >> during early development of CDS archived objects and are no longer relevant. >> >> **Testing:** >> - Mach5 tiers 1 ~ 7 > > cds changes look good! just few nitpicks. Thanks @ashu-mehra @matias9927 @tschatzl for the code review. - PR Comment: https://git.openjdk.org/jdk/pull/13284#issuecomment-1518001489
Integrated: 8298048: Combine CDS archive heap into a single block
On Mon, 3 Apr 2023 03:32:27 GMT, Ioi Lam wrote: > This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1100 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 This pull request has now been integrated. Changeset: 723037a7 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/723037a79d2a43b9a1a247d8f81a47907faadab1 Stats: 3252 lines in 83 files changed: 159 ins; 2446 del; 647 mod 8298048: Combine CDS archive heap into a single block Co-authored-by: Thomas Schatzl Reviewed-by: matsaave, tschatzl - PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v8]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 22 commits: - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - Fixed assert in runtime/cds/appcds/SharedArchiveConsistency.java - Removal of JFR custom closed/open archive region types - Remove g1 full gc skip marking optimization - Some comment updates - Move g1collectedheap archive related regions together in the cpp file - Factor out region/range iteration - Fix comment - Ioi fix - ... and 12 more: https://git.openjdk.org/jdk/compare/f6336231...e8041d50 - Changes: https://git.openjdk.org/jdk/pull/13284/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=07 Stats: 3252 lines in 83 files changed: 159 ins; 2446 del; 647 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8306123: Move InstanceKlass writeable flags
On Tue, 18 Apr 2023 17:11:25 GMT, Coleen Phillimore wrote: > Please review this patch to move the writeable Klass AccessFlags to > InstanceKlassFlags. > Tested with tier1-4. LGTM. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13515#pullrequestreview-1390984891
Re: RFR: 8298048: Combine CDS archive heap into a single block [v7]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits: - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - Fixed assert in runtime/cds/appcds/SharedArchiveConsistency.java - Removal of JFR custom closed/open archive region types - Remove g1 full gc skip marking optimization - Some comment updates - Move g1collectedheap archive related regions together in the cpp file - Factor out region/range iteration - Fix comment - Ioi fix - fixed merge - ... and 11 more: https://git.openjdk.org/jdk/compare/0f3828dd...8a35c7ee - Changes: https://git.openjdk.org/jdk/pull/13284/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=06 Stats: 3252 lines in 83 files changed: 159 ins; 2446 del; 647 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8306282: Build failure linux-arm32-open-cmp-baseline after JDK-8257967 [v3]
On Tue, 18 Apr 2023 15:22:02 GMT, Markus Grönlund wrote: >> Greetings, >> >> With [JDK-8257967](https://bugs.openjdk.org/browse/JDK-8257967), much >> refactoring was done to the JVMTI code concerning agents. However, some >> platforms do not have JVMTI support, and tier5 of testing builds an embedded >> build, linux-arm32-open-cmp-baseline, which failed because the refactoring >> did not properly handle conditional compilations for JVMTI. >> >> JDK-8257967 did run tier5, but it used an existing build, so it did not >> cause recompilations of the embedded target :-( >> >> This changeset adds the conditional constructs to let >> linux-arm32-open-cmp-baseline build successfully. >> >> It does not look good, but there you go... >> >> Testing: >> >> Building: linux-arm32-open-cmp-baseline >> Building: regular platforms >> >> Thanks >> Markus > > Markus Grönlund has updated the pull request incrementally with one > additional commit since the last revision: > > SIZE_FORMAT LGTM - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13512#pullrequestreview-1390388100
Re: RFR: 8298048: Combine CDS archive heap into a single block [v6]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - fixed merge - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - removed G1CollectedHeap::fill_archive_regions() -- we no longer have unused space at the start of the "old" regions - Simplified of runtime range of the mapped archive heap - @ashu-mehra comments; some clean up - @ashu-mehra comments - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - more clean up: heap_regions -> heap_region, etc - @matias9927 comments - Remove archive region types from G1 - ... and 2 more: https://git.openjdk.org/jdk/compare/e3ece365...3543dd2a - Changes: https://git.openjdk.org/jdk/pull/13284/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=05 Stats: 3152 lines in 77 files changed: 121 ins; 2371 del; 660 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block
On Tue, 11 Apr 2023 21:47:40 GMT, Ashutosh Mehra wrote: > cds changes look good! just few nitpicks. Thanks for the review. I've incorporated your suggestions. - PR Comment: https://git.openjdk.org/jdk/pull/13284#issuecomment-1505698259
Re: RFR: 8298048: Combine CDS archive heap into a single block [v5]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @ashu-mehra comments; some clean up - Changes: - all: https://git.openjdk.org/jdk/pull/13284/files - new: https://git.openjdk.org/jdk/pull/13284/files/8ce6953e..30542e53 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=03-04 Stats: 11 lines in 3 files changed: 1 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v4]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @ashu-mehra comments - Changes: - all: https://git.openjdk.org/jdk/pull/13284/files - new: https://git.openjdk.org/jdk/pull/13284/files/b693d27c..8ce6953e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=02-03 Stats: 6 lines in 1 file changed: 0 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v3]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 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 six additional commits since the last revision: - Merge branch 'master' into 8298048-combine-cds-heap-to-single-region-PUSH - more clean up: heap_regions -> heap_region, etc - @matias9927 comments - Remove archive region types from G1 - clean up (1) - 8298048: Combine CDS archive heap into a single block - Changes: - all: https://git.openjdk.org/jdk/pull/13284/files - new: https://git.openjdk.org/jdk/pull/13284/files/a1a3cac7..b693d27c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=01-02 Stats: 33051 lines in 911 files changed: 16125 ins; 14331 del; 2595 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v2]
> This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Ioi Lam has updated the pull request incrementally with two additional commits since the last revision: - more clean up: heap_regions -> heap_region, etc - @matias9927 comments - Changes: - all: https://git.openjdk.org/jdk/pull/13284/files - new: https://git.openjdk.org/jdk/pull/13284/files/a852dfbb..a1a3cac7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=00-01 Stats: 116 lines in 12 files changed: 11 ins; 46 del; 59 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Re: RFR: 8298048: Combine CDS archive heap into a single block [v2]
On Fri, 7 Apr 2023 19:17:46 GMT, Matias Saavedra Silva
wrote:
>> Ioi Lam has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - more clean up: heap_regions -> heap_region, etc
>> - @matias9927 comments
>
> src/hotspot/share/cds/archiveBuilder.cpp line 1086:
>
>> 1084: p2i(to_requested(start)), size_t(end -
>> start));
>> 1085: log_data(start, end, to_requested(start), /*is_heap=*/true);
>> 1086: }
>
> These log messages can be placed inside the else case before the break
Fixed.
> src/hotspot/share/cds/archiveHeapWriter.cpp line 369:
>
>> 367: template void
>> ArchiveHeapWriter::store_requested_oop_in_buffer(T* buffered_addr,
>> 368:
>> oop request_oop) {
>> 369: //assert(is_in_requested_regions(request_oop), "must be");
>
> Some left over commented code. I assume this should be removed or a new
> assert should be here to replace it.
I fixed the assert.
> src/hotspot/share/cds/archiveHeapWriter.cpp line 529:
>
>> 527: num_non_null_ptrs ++;
>> 528:
>> 529: if (max_idx < idx) {
>
> Is there a built in min() function we can use here? Maybe std::min()?
Updated with the `MAX2()` macro.
> src/hotspot/share/cds/filemap.cpp line 1674:
>
>> 1672:
>> 1673: char* buffer = NEW_C_HEAP_ARRAY(char, size_in_bytes, mtClassShared);
>> 1674: size_t written = write_bitmap(ptrmap, buffer, 0);
>
> Maybe add a comment to clarify there is no offset? Constants in method
> parameters can be confusing sometimes.
I changed the code to pass "written" as a parameter similar to the other two
calls. Also added comments.
> src/hotspot/share/cds/filemap.cpp line 2035:
>
>> 2033: }
>> 2034: if (end < e) {
>> 2035: end = e;
>
> Like mentioned before, maybe we have max() and min() methods to use here.
I simplified the code -- there's only one range now so the start/end can be
easily determined.
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 520:
>
>> 518: } else {
>> 519: return true;
>> 520: }
>
> Maybe make this `return reserved.contains(range.start()) &&
> reserved.contains(range.last())`
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163601901
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163601972
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602280
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602339
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602400
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602433
Re: RFR: 8298048: Combine CDS archive heap into a single block
On Sat, 8 Apr 2023 07:14:30 GMT, Thomas Stuefe wrote: > This looks like a nice simplification. Will you also combine all mappings at > OS level to a single one, so that you only need one mmap call? Now there's a single call to mmap() in FileMapInfo::map_heap_regions_impl_inner(). This reminds me that I should remove the "s" from "heap_regions" in the function names. Will do that in my next commit. - PR Comment: https://git.openjdk.org/jdk/pull/13284#issuecomment-1503827701
RFR: 8298048: Combine CDS archive heap into a single block
This PR combines the "open" and "closed" regions of the CDS archive heap into a single region. This significantly simplifies the implementation, making it more compatible with non-G1 collectors. There's a net removal of ~1000 lines in src code and another ~1200 lines of tests. **Notes for reviewers:** - Most of the code changes in CDS are removing the handling of "open" vs "closed" objects. - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). - It might be easier to see the diff with whitespaces off. - There are two major changes in the G1 code - The archived objects are now stored in the "old" region (see g1CollectedHeap.cpp in [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) - The majority of the other changes are removal of the "archive" region type (see heapRegionType.hpp). For ease of review, such code is isolated in [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) - Testing changes: - Now the archived java objects can move, along with the "old" regions that contain them. It's no longer possible to test whether a heap object came from CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. - Many tests that uses this API are removed. Most of them were written during early development of CDS archived objects and are no longer relevant. **Testing:** - Mach5 tiers 1 ~ 7 - Commit messages: - Remove archive region types from G1 - clean up (1) - 8298048: Combine CDS archive heap into a single block Changes: https://git.openjdk.org/jdk/pull/13284/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13284&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8298048 Stats: 2995 lines in 75 files changed: 110 ins; 2271 del; 614 mod Patch: https://git.openjdk.org/jdk/pull/13284.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13284/head:pull/13284 PR: https://git.openjdk.org/jdk/pull/13284
Integrated: 8305421: Work around JDK-8305420 in CDSJDITest.java
On Sun, 2 Apr 2023 22:11:40 GMT, Ioi Lam wrote: > Please review this trivial work-around that removes logging that would > trigger [JDK-8305420](https://bugs.openjdk.org/browse/JDK-8305420). > > I also fixed an incorrect parameter in the `executeAndLog()` call, which > caused the dumping process's stdout/stderr to be logged in the wrong file. This pull request has now been integrated. Changeset: 9ce5fdc9 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/9ce5fdc96262ac80c5a2ac2d51a149408d3d727a Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod 8305421: Work around JDK-8305420 in CDSJDITest.java Reviewed-by: cjplummer - PR: https://git.openjdk.org/jdk/pull/13283
Re: RFR: 8305421: Work around JDK-8305420 in CDSJDITest.java
On Mon, 3 Apr 2023 18:59:03 GMT, Chris Plummer wrote: >> Please review this trivial work-around that removes logging that would >> trigger [JDK-8305420](https://bugs.openjdk.org/browse/JDK-8305420). >> >> I also fixed an incorrect parameter in the `executeAndLog()` call, which >> caused the dumping process's stdout/stderr to be logged in the wrong file. > > Change looks good. Per https://bugs.openjdk.org/browse/JDK-8173304, this is a > known issue and tests need to avoid having the JVM produce too much output > before the debugger connection is setup. Thanks @plummercj for the review. - PR Comment: https://git.openjdk.org/jdk/pull/13283#issuecomment-1495013582
RFR: 8305421: Work around JDK-8305420 in CDSJDITest.java
Please review this trivial work-around that removes logging that would trigger [JDK-8305420](https://bugs.openjdk.org/browse/JDK-8305420). I also fixed an incorrect parameter in the `executeAndLog()` call, which caused the dumping process's stdout/stderr to be logged in the wrong file. - Commit messages: - 8305421: Work around JDK-8305420 in CDSJDITest.java Changes: https://git.openjdk.org/jdk/pull/13283/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13283&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8305421 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13283.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13283/head:pull/13283 PR: https://git.openjdk.org/jdk/pull/13283
Re: RFR: 8228604: StackMapFrames are missing from redefined class bytes of retransformed classes
On Fri, 27 Jan 2023 02:13:28 GMT, David Holmes wrote: > > I'd expect that at least java.util.Date and java.lang.ProcessBuilder have > > the same verification requirement. > > Generally speaking yes - they are both loaded by bootstrap loader and so > would both have verification disabled by default. Bt as you note the > behaviour can change when CDS is involved and only one class gets dumped. > > > But in Date has StackMapTable (starting from JDK12-b15), and ProcessBuilder > > doesn't has StackMapTable. > > This seems odd, but not, IMO, in itself a bug. Perhaps @iklam can comment on > why we treat things differently during dumping. We always enable verification for all classes during -Xshare:dump. That avoid problems where the classes were dumped without verification but at run time you enable verification. - PR: https://git.openjdk.org/jdk/pull/12155
Re: RFC: regarding metaspace(metadata?) dump
On 1/11/2023 6:52 PM, Yi Yang wrote:
Hi Ioi,
> I think there are overlaps between your proposal and existing tools. For
example, there are jcmd options such as VM.class_hierarchy and
VM.classes, etc.
> The Serviceability Agent can also be used to analyze the contents of
the class metadata.
Of course, we can continue to add jcmd commands such as jcmd
VM.method_counter and jcmd VM.aggregtate_by_class_package to help
diagnosing, but another once and for all solution is to implement a
rich and well-formed metadata dump as this proposal described,
third-party parsers and platforms are eligible to analyze well-formed
dump file and provide many grouping/filtering
options(grouping_by_package, filter_linked, filter_force_inline,
essentially VM.class_hierarchy is aggregation of VM.classes).
I'm trying to describe a real use case to illustrate benefits of
well-formed metaspace dump: In our internal DevOps platform, I
observed that the Metaspace utilization rate of my application has
been high. During this period, FGC occurred several times. So I
generate a well-formed metaspace dump through DevOps platform, and
then the dump file will be automatically generated and uploaded to
another internal Java troubleshooting platform, troubleshooting
platform further analyzes and show it with many grouping and filter
options and so on.
> I'd be interested in seeing your implementation and compare it with
the existing tools.
I'm starting to do this, and it may take several months to implement
since it looks more like a JEP level feature, I want to hear some
general discussion before coding, i.e, is it acceptable to use JSON
format? should it be Metadata Dump or keeping the current metaspace
scope? Do you think basic+extend output for internal structure is
acceptable?
Before discussing the output of this tool, I think it's better to first
discuss the goals and intended use
- For Java app developers, I am not sure if they care about the
representation of the classes inside HotSpot. They may want to know what
classes are loaded in what class loaders, or want to trouble shoot
memory leaks (why aren't my classes unloaded, etc). For these, we
already have existing tools.
- For HotSpot developers, it would be nice to have a dump of all the
metadata, but I am not sure how important this is, as people seem to be
able to get by with their own debugging methods.
By the way, there may be multiple ways of creating such a dump. The
least intrusive way would be to program the Serviceability Agent, which
already has a lot of Java APIs to access HotSpot internals. That way,
you can write the dumper without modifying the HotSpot C++ code. It
could even be maintained as a project outside of the JDK repo.
Also you mentioned that "Internally we implemented a metaspace dump that
generates human-readable text". Can you share how this tool was implemented?
Thanks
- Ioi
> This may be quite difficult, because the metadata contains rewritten
Java bytecodes. The rewriting format may be dependent on the JDK
version. Also, the class linkage (the resolution of constant pool
information) will be vastly from one JDK version to another. So using
writing a third party tool that can work with multiple JDK versions
will be quite hard.
Thanks for your input! Maybe display rewrited bytecodes? Anyway, I'll
take a close look at this, and I'll prepare a POC along with dump
parser and a simple UI diagnose web once ready.
> Also, defining a "portable" format for the dump will be difficult,
since we don't know how the internal data structure will evolve in the
future.
Yes, since we don't know how internal data structure will changed in
the future, so I propose reaching a consensus that we can at least
reconstruct Java (rewrited?) source code as much as possible. For
example, the dumped JSON object for InstanceKlass contains two parts,
the first part contains the necessary information to reconstruct the
source code as much as possible, and the second part is extended
information, like this:
{
name:..,
super:..,
flags:...,
method:[]
interface:[]
fields:[],
annotation:[]
bytecode:[],
constantpool:[],
//extend
init_state:...,
init_thread:...,
}
The first part is basically unchanged(or adding new fields only), and
the extended part is subject to change, visualization dump client
checks if fields of JSON objects are defined and displays them further.
--
From:Ioi Lam
Send Time:2023 Jan. 12 (Thu.) 08:15
To:hotspot-runtime-dev ;
serviceability-...@openjdk.java.net
Subject:Re: RFC: regarding metaspace(metadata?) dump
CC-ing serviceability.
Hi Yi,
In general, I think it's good to have tools for understanding the
internal layout of the class metadata layouts.
I think there are overlaps between your proposal and existing
tools. For example, there are jcmd options such as
Re: RFC: regarding metaspace(metadata?) dump
CC-ing serviceability. Hi Yi, In general, I think it's good to have tools for understanding the internal layout of the class metadata layouts. I think there are overlaps between your proposal and existing tools. For example, there are jcmd options such as VM.class_hierarchy and VM.classes, etc. The Serviceability Agent can also be used to analyze the contents of the class metadata. Dd you look at the existing tools and see how they match up with your requirements? I'd be interested in seeing your implementation and compare it with the existing tools. On 1/11/2023 4:56 AM, Yi Yang wrote: Hi, Internally, we often receive feedback from users and ask for help on metaspace-related issues, for example 1. Users are eager to know which GroovyClassLoader loads which classes, why they are not unloaded, and why they are leading to Metaspace OOME. 2. They want to know the class structure of dynamically generated classes in some scenarios such as deserialization 3. Finding memory leaking about duplicated classes ... Internally we implemented a metaspace dump that generates human-readable text, it looks something like this: [Basic Information] Dump Reason : JCMD MaxMetaspaceSize : 18446744073709547520 B CompressedClassSpaceSize : 1073741824 B Class Space Used : 309992 B Class Space Capacity : 395264 B ... [Class Loader Data] ClassLoaderData : loader = 0x8024f928, loader_klass = 0x000800010098, loader_klass_name = sun/misc/Launcher$AppClassLoader, label = N/A Class Used Chunks : * Chunk : [0x00080006, 0x000800060230, 0x000800060800) NonClass Used Chunks : * Chunk : [0x7fd8379c1000, 0x7fd8379c1350, 0x7fd8379c2000) Klasses : Klass : 0x000800060028, name = Test, size = 520 B ConstantPool : 0x7fd8379c1050, size = 296 B ... It has been working effectively for several years and has helped many users solve metaspace-related problems. But a more user-friendly way is that JDK can inherently support this capability. We hope that format of the metaspace dump file can take both flexibility and compatibility into account, and the content of dump file should be detailed enough to meet the needs of both application developers and lower-level developers. Based on above considerations, I think using JSON as its file format is an appropriate solution(But XML or binary format are still not excluded as candidates). Specifically, in earlier thoughts, I thought the format of the metaspace file could be as follows(pretty printed) https://gist.github.com/y1yang0/ab3034b6381b8a9d215602c89af4e9c3 Using the JSON format, we can flexibly add new fields without breaking compatibility. It is debatable as to which data to write. We can reach a consensus that third-party parsers(Metaspace Analyzer Tool) can at least reconstruct Java source code from the dump file. This may be quite difficult, because the metadata contains rewritten Java bytecodes. The rewriting format may be dependent on the JDK version. Also, the class linkage (the resolution of constant pool information) will be vastly from one JDK version to another. So using writing a third party tool that can work with multiple JDK versions will be quite hard. Also, defining a "portable" format for the dump will be difficult, since we don't know how the internal data structure will evolve in the future. Thanks - Ioi Based on this, we can write more useful information for low-level troubleshooting or debugging. (e.g. the init_state of InstanceKlass). In addition, we can even output the native code and associated information with regard to Method, third-party parser can reconstruct the human-readable assembly representation of the compiled method based on dump file. To some extent, we have implemented code cache dump by the way. For this reason, I'm not sure if the title of the RFC proposal should be called metaspace dump, maybe metadata dump? It looks more like a metadata-dump framework. Do you have any thoughts about metaspace/metadata dump? Looking forward to hearing your feedback, any comments are invaluable! Best regards, Yi Yang
Re: RFR: 8286185: The Java manpage can be more platform inclusive [v3]
On Thu, 24 Nov 2022 00:50:22 GMT, David Holmes wrote: >> This is mainly an expansion of the included platforms by changing "linux and >> macOS" to "Non-Windows". There are a few additional examples, and >> clarification that they are just examples. There are also some minor edits >> and corrections I spotted. >> >> One actual fix relates to the "control-break" -> "control-" change. I can >> factor that out if needed (or just add an additional issue to the PR). >> >> This doesn't attempt to give complete platform recognition for all OpenJDK >> platforms. Two areas where anyone interested could file a further RFE is the >> support of DTrace on BSD systems other than macOS; and the use of RTM >> locking on Power8 architecture (existing documentation is all about Intel >> TSX on x86). >> >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Fix formatting LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/11340
Re: RFR: 8295253: Remove kludge from v1_0/PerfDataBuffer.java
On Wed, 23 Nov 2022 19:42:47 GMT, Serguei Spitsyn wrote: >> Remove code for supporting an ancient JDK version (1.4.2). We don't have any >> test cases for this code so it's unclear whether it still works or not. >> >> Testing: tiers 1-4. > > Looks good and reasonable. > Thanks, > Serguei Thanks @sspitsyn @plummercj @dholmes-ora for the review. Passed tiers1-4. - PR: https://git.openjdk.org/jdk/pull/11329
Integrated: 8295253: Remove kludge from v1_0/PerfDataBuffer.java
On Wed, 23 Nov 2022 18:25:59 GMT, Ioi Lam wrote: > Remove code for supporting an ancient JDK version (1.4.2). We don't have any > test cases for this code so it's unclear whether it still works or not. > > Testing: tiers 1-4. This pull request has now been integrated. Changeset: 85ddd8f2 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/85ddd8f2af51fa5ea7f63027285509afb9a5c439 Stats: 149 lines in 1 file changed: 0 ins; 148 del; 1 mod 8295253: Remove kludge from v1_0/PerfDataBuffer.java Reviewed-by: sspitsyn, dholmes, cjplummer - PR: https://git.openjdk.org/jdk/pull/11329
RFR: 8295253: Remove kludge from v1_0/PerfDataBuffer.java
Remove code for supporting an ancient JDK version (1.4.2). We don't have any test cases for this code so it's unclear whether it still works or not. Testing: tiers 1-4. - Commit messages: - 8295253: Remove kludge from v1_0/PerfDataBuffer.java Changes: https://git.openjdk.org/jdk/pull/11329/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11329&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8295253 Stats: 149 lines in 1 file changed: 0 ins; 148 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11329.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11329/head:pull/11329 PR: https://git.openjdk.org/jdk/pull/11329
Re: RFR: 8295315: [REDO] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files
On Fri, 11 Nov 2022 06:13:50 GMT, David Holmes wrote: >> Here's redo for https://github.com/openjdk/jdk/pull/10687. This PR has two >> commits: >> - 739b79afb1965b625b2002187ac3fd43f385a639 is the same as in the original PR >> - 78455c024ec5c00f1a0ce6c0e13df477c3063fe1 fixes the bug in the original PR. >> >> I have run tiers 1 - 4 so hopefully I got it right this time :-) > > Looks good! > > Thanks. Thanks @dholmes-ora @kevinjwalls @sspitsyn for the review. - PR: https://git.openjdk.org/jdk/pull/11094
