Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v12]
On Mon, 23 Sep 2024 17:07:56 GMT, Ioi Lam 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...
>
> 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
Spotted a few nits.
src/hotspot/share/cds/metaspaceShared.cpp line 1536:
> 1534: if (lsh.is_enabled()) {
> 1535: lsh.print("Using AOT-linked classes: %s (static archive: %s
> aot-linked classes",
> 1536: CDSConfig::is_using_aot_linked_classes() ? "true" :
> "false",
Suggestion:
`BOOL_TO_STR(CDSConfig::is_using_aot_linked_classes()),`
test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java line 100:
> 98: TestCommon.checkExec(out);
> 99:
> 100: // JEP 483: if dumped with -XX:+AOTClassLinking, cannot use
> archive when CFLH
Suggestion:
`..., cannot use archive when CFLH is enabled`
test/lib/jdk/test/lib/cds/CDSAppTester.java line 50:
> 48: public CDSAppTester(String name) {
> 49: if (CDSTestUtils.DYNAMIC_DUMP) {
> 50: throw new jtreg.SkippedException("Tests based on CDSAppTester
> should be excluded when -Dtest.dynamic.cds.archive is specified");
You could omit the `jtreg.` in the above throw statement.
-
PR Review: https://git.openjdk.org/jdk/pull/20843#pullrequestreview-2322962760
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1771865540
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1771878531
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1771871838
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Sat, 2 Dec 2023 00:32:30 GMT, Ioi Lam wrote: >> 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. Thanks for the explanation. Looks good then. - PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412714559
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Sat, 2 Dec 2023 00:38:58 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. > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > fixed indentation Marked as reviewed by ccheung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1760771168
Re: RFR: 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. Code migration from arguments cpp to cdsConfig.cpp looks good. Found a minor simplification regarding the include statements. 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. - PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1756281866 PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1409910394
Re: RFR: 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))
One nit in cdsConfig.hpp.
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.
-
Marked as reviewed by ccheung (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16257#pullrequestreview-1688783121
PR Review Comment: https://git.openjdk.org/jdk/pull/16257#discussion_r1366197088
Re: Integrated: 8316695: ProblemList serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java
On Thu, 21 Sep 2023 20:39:32 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java > on all platforms. It's a new test and we already have 8 failure sightings in > the JDK22 CI. Looks good and trivial. - Marked as reviewed by ccheung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15870#pullrequestreview-1638672284
Re: RFR: 8315877: ProblemList vmTestbase/nsk/jvmti/InterruptThread/intrpthrd003/TestDescription.java on macosx-aarch64
On Thu, 7 Sep 2023 19:17:42 GMT, Daniel D. Daugherty wrote: > Trivial fixes to ProblemList some tests: > - [JDK-8315877](https://bugs.openjdk.org/browse/JDK-8315877) ProblemList > vmTestbase/nsk/jvmti/InterruptThread/intrpthrd003/TestDescription.java on > macosx-aarch64 > - [JDK-8315879](https://bugs.openjdk.org/browse/JDK-8315879) ProblemList > java/awt/PopupMenu/PopupMenuLocation.java on macosx-aarch64 Looks good and trivial. - Marked as reviewed by ccheung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15627#pullrequestreview-1616152066
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
On Tue, 14 Mar 2023 20:20:41 GMT, Matias Saavedra Silva
wrote:
>> The current structure used to store the resolution information for
>> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its
>> ambigious fields f1 and f2. This structure can hold information for fields,
>> methods, and invokedynamics and each of its fields can hold different types
>> of values depending on the entry.
>>
>> This enhancement proposes a new structure to exclusively contain
>> invokedynamic information in a manner that is easy to interpret and easy to
>> extend. Resolved invokedynamic entries will be stored in an array in the
>> constant pool cache and the operand of the invokedynamic bytecode will be
>> rewritten to be the index into this array.
>>
>> Any areas that previously accessed invokedynamic data from
>> ConstantPoolCacheEntry will be replaced with accesses to this new array and
>> structure. Verified with tier1-9 tests.
>>
>> The PPC was provided by @reinrich and the RISCV port was provided by
>> @DingliZhang and @zifeihan.
>>
>> This change supports the following platforms: x86, aarch64, PPC, and RISCV
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> RISCV port update
Looks good. Just a few minor comments.
src/hotspot/share/interpreter/bootstrapInfo.cpp line 218:
> 216: _indy_index,
> 217:
> pool()->tag_at(_bss_index),
> 218: CHECK_false);
Please indent lines 216-218 like before.
src/hotspot/share/interpreter/bootstrapInfo.cpp line 234:
> 232: if (_indy_index > -1) {
> 233: os::snprintf_checked(what, sizeof(what), "indy#%d", _indy_index);
> 234: }
Since the `else` case doesn’t have braces, maybe omit the braces for this case
as well?
src/hotspot/share/oops/cpCache.cpp line 618:
> 616: indy_resolution_failed(), parameter_size());
> 617: if ((bytecode_1() == Bytecodes::_invokehandle)) {
> 618: constantPoolHandle cph(Thread::current(), cache->constant_pool());
There is another `cph` defined at line 601. Could that one be used?
src/hotspot/share/oops/cpCache.cpp line 652:
> 650: int size = ConstantPoolCache::size(length);
> 651:
> 652: // Initialize resolvedinvokedynamicinfo array with available data
Maybe breakup the long word `resolvedinvokedynamicinfo`?
src/hotspot/share/oops/cpCache.cpp line 653:
> 651:
> 652: // Initialize resolvedinvokedynamicinfo array with available data
> 653: Array* array;
Suggestion: rename `array` to `resolved_indy_entries`.
src/hotspot/share/oops/cpCache.cpp line 664:
> 662:
> 663: return new (loader_data, size, MetaspaceObj::ConstantPoolCacheType,
> THREAD)
> 664: ConstantPoolCache(length, index_map, invokedynamic_map, array);
I think it reads better if this line is indented to right after the open
parenthesis.
src/hotspot/share/prims/methodComparator.cpp line 119:
> 117: if ((old_cp->name_ref_at(index_old) !=
> new_cp->name_ref_at(index_new)) ||
> 118: (old_cp->signature_ref_at(index_old) !=
> new_cp->signature_ref_at(index_new)))
> 119: return false;
Please adjust the indentations of lines 118 and 119 to be the same as lines 124
and 125.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/BytecodeWithCPIndex.java
line 61:
> 59: } else {
> 60: return cpCache.getEntryAt((int) (0x &
> cpCacheIndex)).getConstantPoolIndex();
> 61: }
Maybe align all `return` statements with line 56?
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyArray.java
line 38:
> 36: public class ResolvedIndyArray extends GenericArray {
> 37: static {
> 38: VM.registerVMInitializedObserver(new Observer() {
Indentation for java code should be 4 spaces.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyEntry.java
line 38:
> 36: private static long size;
> 37: private static long baseOffset;
> 38: private static CIntegerField cpIndex;
Indentation for java code should be 4 spaces.
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8293548: ProblemList sun/management/jmxremote/bootstrap/RmiBootstrapTest.java#id1 on linux-x64
On Thu, 8 Sep 2022 16:19:15 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > sun/management/jmxremote/bootstrap/RmiBootstrapTest.java#id1 on linux-x64. > This time for sure Rocky! I've included the "#id1" sub-test identifier. LGTM - Marked as reviewed by ccheung (Reviewer). PR: https://git.openjdk.org/jdk/pull/10220
Re: RFR: 8289230: Move PlatformXXX class declarations out of os_xxx.hpp [v3]
On Tue, 28 Jun 2022 20:13:01 GMT, Ioi Lam wrote: >> There are only two implementations of these classes (one for windows, and >> one for posix): >> >> - PlatformEvent >> - PlatformParker >> - PlatformMutex >> - PlatformMonitor >> - ThreadCrashProtection >> >> Before this PR, these classes are declared in os_xxx.hpp. This causes >> excessive inclusion of the large header file os.hpp by popular headers such >> as mutex.hpp, which needs only the declaration of PlatformMutex but not the >> other stuff in os.hpp >> >> This PR moves the declarations to park_posix.hpp, mutex_posix.hpp, etc. >> >> Note: ideally, the definition of PlatformParker/PlatformEvent should be >> moved to park_posix.cpp, and PlatformMutex/PlatformMonitor should be moved >> to mutex_posix.cpp. However, the definition of these 4 classes are >> intertwined, so I'll leave them inside os_posix.cpp for now. (Same for the >> Windows version). > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > @coleenp comments Looks good. Just one nit. src/hotspot/share/runtime/mutex.cpp line 334: > 332: } > 333: > 334: Line 334 deleted by accident? - Marked as reviewed by ccheung (Reviewer). PR: https://git.openjdk.org/jdk/pull/9303
