On Tue, 10 Mar 2026 05:49:00 GMT, Thomas Stuefe <[email protected]> wrote:
>> 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. Most of this >> was implemented with JDK-8363998 [3] - here, we just add a compile-time >> switch `INCLUDE_CLASSSPACE`, which is true on 32-bit, false on 64-bit. >> >> 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] https://mail.openjdk.org/pipermail/hotspot-dev/2025-February/101023.html >> [3] https://bugs.o... > > Thomas Stuefe has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 65 commits: > > - Merge branch 'master' into JDK-8363996-Obsolete-UseCompressedClassPointers > - Replace Metaspace::using_class_space with define > - Update src/hotspot/cpu/x86/macroAssembler_x86.hpp > > Co-authored-by: David Holmes > <[email protected]> > - Replace Klass::_metadata union with narrowKlass member > - Ivan: fix various instances of ObjLayout::undefined should assert > - Ivan: Update src/hotspot/share/oops/instanceKlass.cpp > > Co-authored-by: Ivan Walulya <[email protected]> > - David: reduce diff in ObjectCountEventVerifier.java > - David: minimize change in GetObjectSizeIntrinsicsTest.java > - David: minimize diffs in TestZGCWithCDS.java > - David: minimize diffs for > runtime/ErrorHandling/TestVMConfigInHsErrFile.java > - ... and 55 more: https://git.openjdk.org/jdk/compare/9a26b4af...be3a902b src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp line 113: > 111: z_st(len, Address(obj, arrayOopDesc::length_offset_in_bytes())); > 112: } else if (!UseCompactObjectHeaders) { > 113: store_klass_gap(Rzero, obj); // Zero klass gap for compressed oops. The comment has been wrong before and is even more wrong now. src/hotspot/share/code/aotCodeCache.hpp line 181: > 179: debugVM = 1, > 180: compressedOops = 2, > 181: useTLAB = 4, Is it ok to break the numbering? Or is there any expectation of compatibility with older AOT caches? src/hotspot/share/gc/shared/gcTrace.cpp line 126: > 124: send_meta_space_summary_event(when, summary); > 125: send_metaspace_chunk_free_list_summary(when, Metaspace::NonClassType, > summary.metaspace_chunk_free_list_summary()); > 126: send_metaspace_chunk_free_list_summary(when, Metaspace::ClassType, > summary.class_chunk_free_list_summary()); So this would send the JFR MetaspaceChunkFreeListSummary event, even if we have no real class-space on 32-bit? Is this useful, or should we guard this with INCLUDE_CLASS_SPACE instead? src/hotspot/share/memory/metaspace.cpp line 255: > 253: > 254: #if INCLUDE_CLASS_SPACE > 255: // If we use compressed class pointers, verify class chunkmanager... The comment is a bit redundant or misleading. It should say 'If we use class-space' but this is perhaps worse, because it only describes in words what is written in code. So maybe remove it altogether? src/hotspot/share/memory/metaspace/metaspaceReporter.cpp line 279: > 277: > ChunkManager::chunkmanager_nonclass()->add_to_statistics(&non_class_cm_stat); > 278: #if INCLUDE_CLASS_SPACE > 279: > ChunkManager::chunkmanager_nonclass()->add_to_statistics(&non_class_cm_stat); This is pre-existing, but we call ChunkManager::chunkmanager_nonclass()->add_to_statistics(&non_class_cm_stat) twice for INCLUDE_CLASS_SPACE. Is this correct for some reason? Otherwise might be worth fixing while we're here, or file a bug for later. src/hotspot/share/oops/arrayOop.hpp line 83: > 81: // The _length field is not declared in C++. It is allocated after the > 82: // mark-word when using compact headers (+UseCompactObjectHeaders), > otherwise > 83: // after the compressed Klass* Suggestion: // after the compressed Klass*. src/hotspot/share/runtime/os.cpp line 1342: > 1340: > 1341: // Compressed klass needs to be decoded first. > 1342: // Todo: questionable for COH - can we do this better? Is it worth tracking in a JBS issue? src/hotspot/share/services/memoryService.cpp line 122: > 120: _pools_list->append(_metaspace_pool); > 121: > 122: _compressed_class_pool = new CompressedKlassSpacePool(); Does this make sense on 32 bit? Or should it be guarded with INCLUDE_CLASS_SPACE instead? test/hotspot/jtreg/gc/g1/TestSharedArchiveWithPreTouch.java line 69: > 67: > 68: if (Platform.is64bit()) { > 69: dump_args.addFirst("-XX:+UseCompressedOops" ); This should be load_args, right? test/hotspot/jtreg/gtest/ObjArrayTests.java line 30: > 28: > 29: /* @test id=with-coops > 30: * @summary Run object array size tests with compressed oops and > compressed class pointers The test dimension 'compressec class pointers' no longer exists. test/hotspot/jtreg/runtime/cds/appcds/TestZGCWithCDS.java line 118: > 116: out.shouldHaveExitValue(0); > 117: > 118: System.out.println("6. Run with SerialGC, +UseCompressedOops"); The steps numbering is off now. Maybe get rid of the numbering altogether? Or else make it right. test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicArchiveTestBase.java line 309: > 307: * - the VM under test was started with a different options than the > ones > 308: * when the default CDS archive was built. E.g. the VM was started > with > 309: * -XX:+UseZGC which implicitly disables the UseCompressedOoops > option. Suggestion: * -XX:+UseZGC which implicitly disables the UseCompressedOops option. test/jdk/java/lang/instrument/GetObjectSizeIntrinsicsTest.java line 318: > 316: > 317: static final boolean CCP = true; > 318: static final int ARRAY_HEADER_SIZE = CCP ? 16 : (Platform.is64bit() > ? 20 : 16); Get rid of CCP altogether and simplify the ARRAY_HEADER_SIZE to hardcoded 16? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925637203 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925568361 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925592090 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925673870 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925557610 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925645299 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925695827 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925597405 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925537510 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925683703 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925617007 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925677908 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2925607841
