RFR (11u, XXL): Upstream/backport Shenandoah to JDK11u - the review thread

[Roman Kennke]

Hi all,

So I did the exercise that we discussed in the other thread, and wrote
a little script that compares object files of the baseline build with
the object files of a patched builds with Shenandoah disabled. (I will
clean up and post the script for this shortly.)

In order for this to make any sense, I needed to patch debug.hpp to
suppress inclusion of __LINE__ in a bunch of macros. I will clean it up
and turn this into a proper configure flag and propose it for inclusion
soon. I think it might be useful more generally (perhaps for JDK16 and
then trickle down to jdk11u).

This exercise indeed turned up a couple of (harmless) problems where
Shenandoah code did indeed leak into non-Shenandoah builds:
- We had a couple of unguarded UseShenandoahGC clauses in C2 code.
- We generated ShenandoahBarrierNode declarations and node-query in
node.hpp
- ADLC build did not pass -DINCLUDE_SHENANDOAHGC=0 when Shenandoah is
disabled, therefore it would pick up Shenandoah node declarations from
classes.hpp, which in turn would leak into a couple of places like
matcher.o, compile.o, dfa.o and a few others.
- vmOperations.hpp would generate VM_Shenandoah* declarations which
would get compiled into vmOperations.cpp
- JFR generated a couple of Shenandoah related events and types, and
this requires the definition of some empty methods too. I fixed the
generator so that it can spread the JFR metadata.xml over several
files, and include GC specific metadata only on-demand. I will extract
this part and propose it for inclusion upstream in jdk16.

With all those issues fixed, I can (and did) prove that Shenandoah does
not leak anywhere in libjvm.so when Shenandoah is disabled. It does
literally compile identical object files. The only exception is the
inclusion of the global flag UseShenandoah and the code that prints an
error and exists when trying to run with it. (If really needed, I could
actually rip it out too, but it doesn't seem worth, nor very clean).

Shenandoah is in-fact cleaner in this respect than any other GC in
OpenJDK. :-)

There are two places left where Shenandoah has potential impact to
shared code:
- Serviceability-agent: we needed to patch some Java code in order to
provide support for Shenandoah in SA. As far as I can tell, it is dead
code when running in a Shenandoah-disabled build. (In-fact, it should
actually work to use the resulting JAR with a build that has Shenandoah
enabled ;-).
- Tests: a couple of tests have been amended with Shenandoah
configurations. All those new configurations are disabled and will not
be run in a build with Shenandoah disabled. This is also relatively
easy to prove by actually running the affected tests with Shenandoah-
disabled builds.

Shared-code changes only:
http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.09-shared/

Full webrev:
http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.09-all/

What do you think?

Roman


On Thu, 2020-07-09 at 19:36 +0200, Roman Kennke wrote:
> I'm starting a new thread with a new round of webrevs. Maybe we can
> keep in-principle discussions on the other thread and get some actual
> reviews here?
> 
> Anyhow, small changes since the last one:
> - the lone include "interpreter.hpp" is gone, apparently it was a
> left-
> over that's no longer needed
> - the 2 friend class declarations are now wrapped in
> INCLUDE_SHENANDOAHGC too, just to be sure (even though compiler
> wouldn't compile any actual code)
> 
> As noted in the other thread, there should not be any Shenandoah code
> leaking out when building with --with-jvm-features=-shenandoahgc
> (which
> is the default too). There's 2 exceptions:
> 
> - The global UseShenandoahGC flag. When invoked but not built-in, it
> will print an error message and exit.
> - In:
> src/hotspot/share/jfr/periodic/jfrPeriodic.cpp
> 
> I could not exclude the definition of:
> TRACE_REQUEST_FUNC(ShenandoahHeapRegionInformation)
> 
> Because that would be called by generated code. Related to this:
> 
> src/hotspot/share/jfr/metadata/metadata.xml
> 
> This generates two event classes for Shenandaoh, but they would not
> be
> used. I don't know how to exclude them. It seems harmless to me.
> 
> Shared-only changes:
> http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.07-shared/
> 
> Full webrev:
> http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.07-all/
> 
> 
> Roman
>