On Mon, 8 Dec 2025 04:39:52 GMT, David Holmes <[email protected]> wrote:
>> 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]> > > 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? It is. using_class_space is true on LP64, false on 32-bit. Mostly a matter of taste. I kept the function here since it conveys more information than just a blank ifdef. And this approach (if uses as negation) minimizes compile errors on the other side since nobody bothers to build on 32-bit anymore. I have no strong emotions, though. If reviewers prefer it, I'll replace it with a blank ifdef _LP64. > 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. ??? I fixed the comment. I also removed the last sentence (about placing the class space atop of the java heap) since that is not true anymore since https://bugs.openjdk.org/browse/JDK-8312018 . > 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. Reverted > 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. ?? Well, sure. Most of these combinations vanish if you remove compressed oops from the equation. Since it is a constant, it cannot differ between runtime and dumptime, which is what this test checks. It leaves compressed oops, which can differ, so the test is still not empty. > 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. I rewrote the changes; apart from the string constants (must have been my IDE), most of the changes made sense, though. With the removal of CompressedClassPointers switch, fewer combinations are to be tested. I removed all that tested -UseCompressedClassPointers, and removed resulting duplicates with +UseCompressedClassPointers. Unrelated, I also removed the last check (Step 8) because it duplicates Step 1. Unrelated and preexisting: The test does not check various combinations of UseCompactObjectHeaders, but it should. I opened https://bugs.openjdk.org/browse/JDK-8377454 to track that separately. > 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. Okay. Array header size was wrong for 32-bit; just factoring CCP==true would have given a hard-coded "16" bytes, always, which seemed off, so I just fixed it. Doesn't matter for the test; just a clarity issue. I will reverted the change and opened https://bugs.openjdk.org/browse/JDK-8377456 to track this. > 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. I reduced the diff. Note that the reason for the change is that the tests using this utility functions all ran with -UseCompressedClassPointers for whatever reason (I assume the tests predate the class space and that when class space was introduced the simplest way to fix the test was to run them without class space). I changed the tests to run with +UseCompressedClassPointers, so I need to change this calculation here, too. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2780955585 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2781764089 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2781767546 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2781466821 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2781791263 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2781723738 PR Review Comment: https://git.openjdk.org/jdk/pull/28366#discussion_r2781992992
