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

[Roman Kennke]

And here comes the patch & script to verify the builds:

1. We need a patch to disable line numbers:
http://cr.openjdk.java.net/~rkennke/disable-linenums.patch

I opted to not change any build machinery and instead simply disable
line numbers by configuring with --with-extra-cflags=-
DDISABLE_LINE_NUMBERS

This patch needs to be applied *both* for the baseline and patched
build that the below script compiles.

2. The UseShenandoahGC flag should be included in both baseline and
patched build, for the sake of this comparison, otherwise it will
generate a bunch of extern symbols all over the place:

http://cr.openjdk.java.net/~rkennke/shenandoahflag.patch

3. The actual big Shenandoah patch:

http://cr.openjdk.java.net/~rkennke/shenandoah.patch

(The webrev.09-all combines both Shenandoah patches.)

4. The script that builds with and without the Shenandoah patch and
compares the resulting object files:

http://cr.openjdk.java.net/~rkennke/compare-builds.sh

Place this in the root of the jdk11u source directory.

I am using Mercurial patch queues to handle the patches. hg qimport the
patches above:

hg qimport http://cr.openjdk.java.net/~rkennke/disable-linenums.patch
hg qpush
hg qimport http://cr.openjdk.java.net/~rkennke/shenandoahflag.patch
hg qpush
hg qimport http://cr.openjdk.java.net/~rkennke/shenandoah.patch

Don't apply the big patch just yet. The compare script will do that for
you.

Invoke the script:
./compare-builds.sh

It will make a clean build into build/baseline (including disable-
linenums.patch and shenandoahflag.patch), and a patched build into
build/patched (including all 3 patches), and compare the resulting
object files.

You should see that it complains about vm_version.o, that is because it
captures the time-stamp of the build (it can be verified by objdump -s
both objects and diffing the outputs).

The remaining failure in gcConfig is the code that prints out the error
and exists when invoked with -XX:+UseShenandoahGC (it could perhaps be
extracted into shenandoahflag.patch with some more work).

(For the record, I am doing this experiment with gcc 8.3.1. YMMV with
other compilers)

If you are trying this too, let me know how it goes.

Roman


On Mon, 2020-07-13 at 18:06 +0200, Roman Kennke wrote:
> 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
> >