RFR: JDK-8240224 Allow building hotspot without the serial gc

[Kim Barrett]

> On Mar 10, 2020, at 10:53 AM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
> 
> On 2020-03-09 21:37, Kim Barrett wrote:
>>> On Mar 9, 2020, at 4:30 AM, Magnus Ihse Bursie <magnus at ihse.net>
>>>  wrote:
>>> 
>>> When reworking the JVM feature handling, I wanted to try to compile Hotspot with various features enabled/disabled. I quickly found out that it's not really possible to build hotspot without the serial gc. While this is not a terribly important use case, I think it's good to be able to select serial freely, just as with the other collectors.
>>> 
>>> With this patch it is possible to build a truly minimal JVM using 'configure --with-jvm-variants=custom --with-jvm-features=g1gc'.
>>> 
>>> Bug: 
>>> https://bugs.openjdk.java.net/browse/JDK-8240224
>>> 
>>> WebRev: 
>>> http://cr.openjdk.java.net/~ihse/JDK-8240224-building-without-serial-gc/webrev.01
>>> 
>>> 
>>> /Magnus
>>> 
>> I'm inclined to agree with David and Aleksey that this isn't really a
>> worthwhile exercise. Especially not if it involves making some
>> otherwise questionable or controversial changes.
>> 
> 
> As I've said in the previous comments, it's not so much about making Hotspot running without Serial GC as making configure live up to it's promise not to create an un-buildable configuration.

The ability to configure which GCs are present was added for several
reasons.

Some packagers don't want to support some of the collectors that are
available in the source tree, so want to completely exclude the (to
them) unsupported collectors from their builds.

Some packagers want to be able to reduce the VM footprint for certain
application areas; the "minimal" variant is an example.

In preparation for removal of CMS it was useful to first be able to
build with it configured out.  And CMS could have ended up in the
category of collectors that are excluded as unsupported by some
packagers.

The implementation of this configurability tried to be reasonably
complete.  Doing so helped shake out problems and show the intent.
But I don't know if it was ever demonstrated to work for all
possibilities, and even if it did at one time, bit rot is pretty much
inevitable since we don't test most of those possibilities.

I don't think we should be spending effort on configurations for which
there is no evidence anyone actually wants or needs them.  But having
the mechanism in the build system to try a configuration provides a
starting point if someone finds a need for something oddball, even if
it doesn't work out of the box.  It would be better if broken
configurations failed nicely, but even that can't be ensured for long
without ongoing testing that I don't think anyone wants to do.

> I apologize if my changes are questionable or controversial -- my assessment was on the contrary that they were simple and non-obtrusive, to the point of triviality.

Some of the discussion in this thread has been pointing out places
where a reviewer thinks that assessment is mistaken.

>> src/hotspot/share/gc/shared/gcConfig.cpp
>> 
>> I would instead suggest there should not be a default at all instead
>> of adding these cases, and the user must explicitly select the GC to
>> be used. Since we're talking about an atypical custom build anyway,
>> the user presumably knows what they are doing. And yeah, that makes
>> the buildjdk stuff elsewhere in this patch harder.
>> 
> 
> If you build without the Serial GC, it is not even possible to start the JVM without a flag selecting GC. Instead you get a somewhat cryptic (and incorrect) message about missing garbage collectors. Even if the end user would be able to know that you need to pass an additional option just to be able to start java, the build system knows no such thing, so we cannot even finish the build -- as soon as we try to use the newly built JVM (e.g. for running jlink), we will crash and burn.

Right, because the build system isn't dealing with the need to
explicitly specify the GC to use in such a configuration.  That's what
I meant about making the build stuff harder.  The build system would
need to look at the configuration to decide how to accomplish the build.

>> src/hotspot/share/gc/shared/genCollectedHeap.cpp
>>  197 #if INCLUDE_SERIALGC
>>  198   MarkSweep::initialize();
>>  199 #endif
>> 
>> This whole file, and several associated files, are *only* used by
>> SerialGC now that CMS has been removed: JDK-8234502.
>> 
> 
> Then maybe they should be excluded when serial is not included?

That would be part of the work involved in resolving JDK-8234502.

> Or, if it is determined that Serial GC is essential to hotspot, we should remove the INCLUDE_SERIALGC define and associated framework, since it's just a fake abstraction if it is not actually possible to build without serial GC.

I don’t think there is any belief that SerialGC must always be included.  That it can’t
currently be excluded is an artifact of nobody having the need and the resources to
make that possible.

>> make/hotspot/lib/JvmFeatures.gmk 
>>   58 ifeq ($(JVM_VARIANT), custom)
>>   59   JVM_CFLAGS_FEATURES += -DVMTYPE=\"Custom\"
>>   60 endif
>> 
>> This change looks unrelated to whether serialgc is present or absent.
>> If so, it doesn't belong in this changeset at all.
>> 
> 
> You are correct that this is not strictly about serialgc. When I tested my custom build with only epsilongc, I discovered that jtreg barfed on the version string produced by the custom JVM build. This is a fix that makes sure the VMTYPE always has a value. If you object to me pushing it as part of this fix, I can remove it from here and submit it as a separate issue. (I just didn't think it was worth the hassle.)

I understand there is overhead to breaking things into multiple
changes, but combining unrelated changes can make archeology and
problem or rationale attribution much harder.  I looked at this and
had no idea what it was for, and it wasn't called out in the RFR or
anywhere else.

>> make/hotspot/lib/JvmFeatures.gmk 
>> [removed]
>>  154   # If serial is disabled, we cannot use serial as OldGC in parallel
>>  155   JVM_EXCLUDE_FILES += psMarkSweep.cpp psMarkSweepDecorator.cpp
>> 
>> This was missed by JDK-8235860, which removed those files.  Good find.
>> 
> ... but according to your comment above, that fix also missed to add a bunch of other files that should be excluded..? (If we should keep the ability to disable serial gc, that is…)

The comment above was about a different change, the removal of CMS,
which is known to be incomplete and have a number of further cleanups
and refactorings to do before all vestiges have been removed.

This one is about the removal of the Serial-Old variant of ParallelGC,
which was thought to be complete, but missed this little snippet.

>> test/hotspot/gtest/gc/shared/test_collectorPolicy.cpp
>> 
>> As originally written, this test was *only* testing SerialGC. It's not
>> obvious that it is actually GC-agnostic and can use the default GC if
>> that isn't SerialGC.  Certainly some of the naming suggests otherwise.
>> Was this tested with all the other configurations?
>> 
> 
> No, I have not tested all other configurations. I verified that I could build with only g1, only zgc and only epsilongc. I also tested to run tier1 testing, and it "mostly" succeeded, but it still failed on several tests. My quick eyeballing of the situation indicated that the absolute majority (and perhaps all) these failures were related to jtreg tests not properly declaring their dependencies on compiler1 or compiler2. (Remember, on this bare-bones JVM I only had the interpreter, and neither c1 nor c2).
> 
> I *could* of course run a suitable set of testing with say c1 and c2 enabled, and just a single gc enabled, for the set of all gcs != serial gc, but then we're *really* getting into the "not worth it" land.
> 
> It is not clear to me that the test is only run with Serial GC. As far as I can interpret the test framework, this is run with the default collector, which typically is *not* serialgc on our testing framework. If this is only valid for Serial GC, perhaps the test needs to be amended?

Looking at this some more, I don't know what this test thinks it's
doing, but I suspect it's confused.  It's using TEST_VM and
TEST_OTHER_VM, both of which create the VM before running the test
body.  The kinds of things it's doing in that context seem pretty
questionable.