8282729: Serial: Move BOT implementation to collector specific directory

[xmas92]

Moves serial code from share/gc/shared/blockOffsetTable.* to share/gc/serial/serialBlockOffsetTable.*
Move required fixing up some missing header files.
Refactoring tries to respect INCLUDE_SERIALGC without changing any behaviour.

Given that --disable-jvm-feature-serialgc will not build before this change, it will not build afterwards either. However it does now compile, but will fail to link.

The diff below is enough to for make hotspot to run (on macosx-aarch64), but it will not build the complete JDK as the build system will use the compiled binaries with -XX:+UseSerialGC which obviously won't work as that GC is missing.

diff --git a/src/hotspot/share/gc/shared/genCollectedHeap.cpp b/src/hotspot/share/gc/shared/genCollectedHeap.cpp
index 5558f1a4a32..79b465b3e0a 100644
--- a/src/hotspot/share/gc/shared/genCollectedHeap.cpp
+++ b/src/hotspot/share/gc/shared/genCollectedHeap.cpp
@@ -31,7 +31,6 @@
 #include "code/icBuffer.hpp"
 #include "compiler/oopMap.hpp"
 #include "gc/serial/defNewGeneration.hpp"
-#include "gc/serial/markSweep.hpp"
 #include "gc/shared/adaptiveSizePolicy.hpp"
 #include "gc/shared/cardTableBarrierSet.hpp"
 #include "gc/shared/cardTableRS.hpp"
@@ -80,6 +79,9 @@
 #if INCLUDE_JVMCI
 #include "jvmci/jvmci.hpp"
 #endif
+#if INCLUDE_SERIALGC
+#include "gc/serial/markSweep.hpp"
+#endif
 
 GenCollectedHeap::GenCollectedHeap(Generation::Name young,
                                    Generation::Name old,
@@ -204,9 +206,9 @@ void GenCollectedHeap::post_initialize() {
   initialize_size_policy(def_new_gen->eden()->capacity(),
                          _old_gen->capacity(),
                          def_new_gen->from()->capacity());
-
+#if INCLUDE_SERIALGC
   MarkSweep::initialize();
-
+#endif
   ScavengableNMethods::initialize(&_is_scavengable);
 }
 
diff --git a/test/hotspot/gtest/gc/shared/test_collectorPolicy.cpp b/test/hotspot/gtest/gc/shared/test_collect
orPolicy.cpp
index dfe096248c7..30a24fd9232 100644
--- a/test/hotspot/gtest/gc/shared/test_collectorPolicy.cpp
+++ b/test/hotspot/gtest/gc/shared/test_collectorPolicy.cpp
@@ -31,6 +31,7 @@
 #include "utilities/macros.hpp"
 #include "unittest.hpp"
 
+#if INCLUDE_SERIALGC
 class TestGenCollectorPolicy {
  public:
 
@@ -279,3 +280,4 @@ TEST_OTHER_VM(CollectorPolicy, old_cmd) {
 
   TestGenCollectorPolicy::TestWrapper::test(&setter_old_size, &setter_max_new_size, &checker_large);
 }
+#endif

There is still a lot of Serial only code in the shared code base. The share/gc/shared/space.* is a clear next candidate for refactoring as this change required a bunch of code in there to depend on INCLUDE_SERIALGC to build.

Testing: Tier 1

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8282729: Serial: Move BOT implementation to collector specific directory

Reviewers

• Thomas Schatzl (@tschatzl - Reviewer)
• Albert Mingkun Yang (@albertnetymk - Reviewer) ⚠️ Review applies to ffd049f3

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9662/head:pull/9662
$ git checkout pull/9662

Update a local copy of the PR:
$ git checkout pull/9662
$ git pull https://git.openjdk.org/jdk pull/9662/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9662

View PR using the GUI difftool:
$ git pr show -t 9662

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9662.diff

[bridgekeeper]

👋 Welcome back xmas92! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@xmas92 The following label will be automatically applied to this pull request:

• hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (a31fb7fc)
• 01: Full - Incremental (ffd049f3)
• 00: Full (b6a62121)

[tschatzl]

Can you check tier4/tier5, which includes the serviceability tests after these changes (just do tier1-5 to be safe).

Looks good apart from this request to rerun testing.

[xmas92]

Passed: tier 2-5
java/lang/ProcessBuilder/PipelineLeaksFD.java failed in tier 1. But it succeeded in the previous run (same source) and succeeded when I reran tier 1. Given that the test is working with os pipes it seems unrelated to this PR.

[tschatzl]

Thanks.

[openjdk]

@xmas92 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8282729: Serial: Move BOT implementation to collector specific directory

Reviewed-by: tschatzl, ayang

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 48 new commits pushed to the master branch:

• 486f90c: 8292716: Configure should check that jtreg is of the required version
• b4e1aa8: 8289658: Avoid redundant LinkedHashMap.get call in TagletManager.addNewSimpleCustomTag
• 84936e9: 8292586: simplify cleanups in NTLMAuthSequence getCredentialsHandle
• 259ba86: 8290075: [JVMCI] only blessed methods can link against EventWriterFactory.getEventWriter
• a85a723: 8292351: tty should always live
• 4da1745: 8291118: [vectorapi] Optimize the implementation of lanewise FIRST_NONZERO
• 38a8191: 8290322: Optimize Vector.rearrange over byte vectors for AVX512BW targets.
• 27af014: 8292743: Missing include resourceHash.hpp
• f58aaab: 8292262: adjust timeouts in several M&M tests
• ab69885: 8292215: java/util/stream/boottest/java.base/java/util/stream/SpinedBufferTest.java times out with slowdebug
• ... and 38 more: https://git.openjdk.org/jdk/compare/f2f0cd86bf4dce4633f484476077fd090549780e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@tschatzl, @albertnetymk) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[albertnetymk]

It's a bit surprising to see changes to other collectors in a commit titled "Serial: ...". I guess that's what you meant by "Move required fixing up some missing header files." I wonder if you can extract those fixes in another PR(s), which, IMO, would also make reviewing this PR slightly easier. Basically keeping each PR more focused.

[xmas92]

It's a bit surprising to see changes to other collectors in a commit titled "Serial: ...". I guess that's what you meant by "Move required fixing up some missing header files." I wonder if you can extract those fixes in another PR(s), which, IMO, would also make reviewing this PR slightly easier. Basically keeping each PR more focused.

It seems a bit ugly to leave the repository in an intermediate state where some headers are included only because there is something, somewhere else, that need that include. But if it makes the review easier I'll move

-#include "gc/shared/gc_globals.hpp"
-#include "memory/virtualspace.hpp"

into a different issue, so G1 and Epsilons still have all their required (indirect) includes.

[albertnetymk]

Thank you for the update. Now I can use git diff --color-moved=dimmed_zebra to ignore pure "moves".

I actually think BlockOffsetTable.hpp is better than serialBlockOffsetTable.hpp, as it matches the type name. The counterpart would be ObjectStartArray in the Parallel collector, which also has a relatively long name, unlike G1.

Either way, this PR is good to go.

[xmas92]

I agree, used that name because of the All source files must have a globally unique basename. The build system depends on this uniqueness. rule.
But it will be cleaner if the BlockOffsetTable.hpp still in shared would be named something like BOTConstants.hpp or BlockOffsetTableConstants.hpp. However that change would change the header includes in a lot of files, but as it is only a rename maybe it is ok? From a review perspecive.

Regardless want to integrate #9921 / JDK-8292606 before this so the header files in whatever shared/BlockOffsetTable.hpp will be called can be cleaned up.

[tschatzl]

Still good.

[xmas92]

/integrate

[openjdk]

@xmas92
Your change (at version a31fb7f) is now ready to be sponsored by a Committer.

[albertnetymk]

/sponsor

[openjdk]

Going to push as commit 0813a47.
Since your change was applied there have been 59 commits pushed to the master branch:

• fa5cc4c: 8291878: NMT: Malloc limits
• ad2e0c4: 8292778: EncodingSupport_md.c convertUtf8ToPlatformString wrong placing of free
• 13c0369: 8242181: [Linux] Show source information when printing native stack traces in hs_err files
• fe0544f: 8292790: Remove hashtable.hpp from other files
• 926380d: 8292640: C2: Remove unused scratch register usages on x86
• f3be673: 8292638: x86: Improve scratch register handling in VM stubs
• d24b7b7: 8276651: java/lang/ProcessHandle tests fail with "RuntimeException: Input/output error" in java.lang.ProcessHandleImpl$Info.info0
• 8a45abd: 8292654: G1 remembered set memory footprint regression after JDK-8286115
• cc28783: 8292777: Remove hashtable.hpp from dependencies.hpp
• cf00677: 8292368: [ppc64] internal error g1BarrierSet.inline.hpp assert(oopDesc::is_oop(pre_val, true)) failed: Error
• ... and 49 more: https://git.openjdk.org/jdk/compare/f2f0cd86bf4dce4633f484476077fd090549780e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@albertnetymk @xmas92 Pushed as commit 0813a47.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.