On Thu, 4 Dec 2025 13:36:43 GMT, Thomas Stuefe <[email protected]> wrote:
>> _This patch is not intended for JDK 26_. >> >> I'm posting it now to collect feedback and, barring any objections, plan to >> push it once JDK 27 opens. >> >> This change removes the uncompressed Klass pointer mode and, with compressed >> Klass pointers remaining as the only option, the >> `UseCompressedClassPointers` switch. >> >> For motivation, please take a look at CSR associated with the deprecation >> (which we did for JDK 25) and the preparatory discussion we had at the start >> of the year around this topic [2]. >> >> This patch is quite invasive and touches many parts of the JVM, since its >> goal is to remove most traces of the uncompressed Klass path and to take >> advantage of opportunities for simplification. In some cases, I did not take >> opportunities for further simplification to keep the patch somewhat legible; >> it will be onerous enough to review. >> >> ### Implementation Notes >> >> With uncompressed Klass pointers removed, we have three modes of operation >> left (including 32-bit): >> a) 64-bit, COH off - this is the old `+UseCompressedClassPointers` mode. >> This is now the standard mode until we run with COH by default. >> b) 64-bit, COH on >> c) 32-bit - Here, we run with a "fake" narrow Klass pointer mode. We run >> with hardcoded narrowKlass base == NULL and shift = 0, so nKlass == Klass*. >> The difference to (a, b) is that we don't use a class space. This was >> implemented with JDK-8363998 [3] - for more details, please see that issue >> and its PR. >> >> I ensured *arm32* builds and I performed some rudimentary checks (selected >> metaspace/gc tests, and a simple Spring PetClinic run). Vendors with an >> interest in arm32 will have to step up and do their own, more thorough unit >> testing. Also, I did not see anyone doing follow-up work after JDK-8363998 >> [3] - so some issues may still lurk from that patch as well (but maybe >> JDK-8363998 was just not breaking anything). >> >> I did not check *zero 32-bit*, the only other platform supporting 32-bit. >> Anyone with an interest in 32-bit zero should chip in. >> >> Pre-existing errors: While working on this patch, I stumbled over a few >> occurrences of old but benign bugs. Mostly old code assuming >> CompressedClassPointers and CompressedOops were still tied together >> (example: Arguments::set_heap_size()). These bugs are implicitly fixed with >> this patch. >> >> ### Testing >> >> - tier 1 2 3 locally on Linux x64 >> - SAP ran their whole set of tests for all the platforms they support. >> >> >> [1] https://bugs.openjdk.org/browse/JDK-8350754 >> [2... > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > Update src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp > > Co-authored-by: Andrew Haley <[email protected]> I've completed a pass through everything and have a couple of comments on the functional changes. The tests were quite hard to follow in places and I've flagged a few where you seemed to have done a more extensive rewrite than what was actually needed for the obsoletion - making it hard to assess the changes. Thanks src/hotspot/share/memory/metaspace.cpp line 661: > 659: MaxMetaspaceSize = MAX2(MaxMetaspaceSize, commit_alignment()); > 660: > 661: if (using_class_space()) { Shouldn't this now just be a build-time `ifdef _LP64` check? src/hotspot/share/memory/metaspace.cpp line 728: > 726: } > 727: > 728: // a) if CDS is active (runtime, Xshare=on), it will create the class > space You have left descriptions of a) and b) but now we have no idea what they are referring to. ??? src/hotspot/share/runtime/arguments.cpp line 1591: > 1589: > 1590: size_t heap_end = HeapBaseMinAddress + MaxHeapSize; > 1591: size_t max_coop_heap = max_heap_for_compressed_oops(); Please don't change the types in this PR. test/hotspot/jtreg/runtime/CompressedOops/CompressedClassPointers.java line 294: > 292: } > 293: > 294: public static void heapBaseMinAddressTestNoCoop() throws Exception { It is not obvious to me why this test case was deleted. test/hotspot/jtreg/runtime/ErrorHandling/TestVMConfigInHsErrFile.java line 59: > 57: switch (args[0]) { > 58: case "coh-on" -> testCompactObjectHeaders(); > 59: case "coh-off" -> testCompressedClassPointers(); You seem to have done a complete test rewrite here and it is not obvious to me it follows as part of the obsoletion of UCCP. test/hotspot/jtreg/runtime/cds/appcds/TestCombinedCompressedFlags.java line 67: > 65: private void initExecArgs() { > 66: // We fail this test if the UseCompressedOops setting used at > dumptime differs from runtime, > 67: // succeed if it is identical This test seems to reduce to pretty much nothing. ?? test/hotspot/jtreg/runtime/cds/appcds/TestZGCWithCDS.java line 44: > 42: private final static String UNABLE_TO_USE_ARCHIVE = "Unable to use > shared archive."; > 43: private final static String ERR_MSG = "The saved state of > UseCompressedOops (0) is different from runtime (1), CDS will be disabled."; > 44: private final static String COMPACT_OBJECT_HEADERS = > "-XX:+UseCompactObjectHeaders"; Again this test seems to have a lot of unrelated changes. This is obscuring the actual differences and makes the review harder. test/jdk/java/lang/instrument/GetObjectSizeIntrinsicsTest.java line 318: > 316: > 317: // Includes length, excludes alignment gap to base > 318: static final int ARRAY_HEADER_SIZE = Platform.is64bit() ? 16 : 12; Again unrelated change. test/jdk/jdk/jfr/event/gc/objectcount/ObjectCountEventVerifier.java line 75: > 73: final int bytesPerWord = runsOn32Bit ? 4 : 8; > 74: final int objectHeaderSize = runsOn32Bit ? 12 : 16; > 75: final int alignmentGap = runsOn32Bit ? 4 : 0; Again unrelated changes. ------------- PR Review: https://git.openjdk.org/jdk/pull/28366#pullrequestreview-3550086639 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597013211 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597014493 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597023027 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597045861 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597050708 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597102266 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597107079 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597110703 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2597114451
