RFR (11u, XXL): Upstream/backport Shenandoah to JDK11u

[Roman Kennke]

On Wed, 2020-07-08 at 21:39 +0000, Gil Tene wrote:
> > On Jul 8, 2020, at 11:31 AM, Roman Kennke <rkennke at redhat.com>
> > wrote:
> > 
> > Hi Gil & all,
> > 
> > I put in some extra work and had a deep look at the remaining
> > unguarded
> > changes in shared-code, with an eye to either eliminating them
> > altogether, or - where this is not possible - guard them with
> > build-
> > time #if INCLUDE_SHENANDOAHGC or similar. The guiding principle in
> > this
> > effort has been that building without Shenandoah (--with-jvm-
> > features=-
> > shenandoahgc) would compile *the exact same* code that it does now.
> > I
> > believe I achieved this with the following proposed changes:
> > 
> > Full webrev (incl. Shenandoah-only files):
> > https://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.06-all/
> > 
> > Shared-code-only changes:
> > https://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.06-shared/
> > 
> > The latter webrev exludes all files that are only compiled with
> > Shenandoah enabled.
> > 
> > Also, compared to previous webrevs, this excludes Shenandoah by
> > default.
> > 
> > As far as I can tell, this literally compiles the same code into
> > libjvm.so as it does now, when building without Shenandoah. The
> > only
> > exception is the inclusion of the global flag UseShenandoahGC,
> > which is
> > by-design. When this is enabled in a build without Shenandoah, it
> > prints the appropriate error message and exist (exactly like with
> > any
> > other GC that is not built-in).
> 
> For the purpose of this thought exercise, I think we could consider
> that flag
> addition as a separate change (which would happen before all the
> other
> stuff). So that we can consider the "would compile *the exact same*
> code"
> statements in the context of a builds that already include the flag.
> 

Ok.

> > I've been thinking about if we can come up with a mechanical proof
> > of
> > correctness. The first best thing would have been reproducible
> > builds,
> > but we don't have that (and the UseShenandoahGC flag would mess it
> > up
> > too). The other option would be to dump preprocessed sources and
> > diff
> > them, but that is disturbed by the necessary inclusion of
> > "utilities/macros.hpp" which gives us the INCLUDE_SHENANDOAHGC
> > macro.
> 
> I think that a mechanized way to verify the assumption will be key,
> as you
> will want it verified not just initially, but with every future
> change. figuring
> out how to do that is a key question...
> 
> Yes, reproducible builds would be great, but we probably don't want
> to
> hold our breath for that. Maybe something around comparing individual
> object file contents that go into the final link can work?
> 

Maybe. I'm not familiar enough with object file formats to see quickly
how to do that, or if it's even feasibible

> > Which leaves us with carefully inspecting the webrev by multiple
> > sets
> > of eyes and brains.
> 
> Eye and brain inspection is unlikely to catch innocent looking
> "leaks" in
> this model 6 months after the initial merge effort is completed, when
> someone touches a line on the wrong side of an ifdef, or makes a
> change to the scope of an ifdef or to what turns it on it.
> 
> I think that an ongoing means of verification is needed as part of a
> solution like this for it to be viable maintainable. I'm not sure
> what
> that would look like. But it may be worth thinking up some
> techniques.

Sorry I don't understand. Why is the usual practices of backporting,
reviewing, testing for jdk11u etc not good enough to police ongoing
work?

>From our long experience with backporting both non-Shenandoah and
Shenandoah stuff to 11u, we know that they rarely, if ever, come into
conflict with each other, and it's usually quite clear what is what. 

> I'm also not sure how one would police future code changes and
> backports of fixes from usptream versions, unless each and every
> review of code carefully takes into consideration the "is this
> Shenandoah
> related or not? Should it go in ifdefs or not?" questions.

Easy: if the change has 'gc-shenandoah' label in the bug report, it's
Shenandoah, otherwise it's not. :-) We are very careful to get that
part right for our own tracking. Shared-code is very rarely touched by
Shenandoah changes anyway.

> Can we come up with systemic ways to know that a code change being
> backported is Shenandoah-related? E.g. if the upstream code shape and
> ifdef'ing rules remained similar to the downstream,

The ifdef'ing rules are already different: there are (almost) no such
#ifdefs in later JDKs, because it's all isolated by the proper GC
interfaces. Most of those exist in 11u, but not all, that's why we had
to put those #ifs in the patch. Some stuff has been accepted as general
change in later JDKs (e.g. the stuff in ciInstanceKlass.cpp), it
appears as Shenandoah change here, because we are the only users of it.

>  we may be able to
> keep things workable, but I suspect trhat the upstream (especially
> after
> 15u) will be tempted to remove the strict ifdef'ing pretty quickly,
> and start
> placing code changes in common code, and as those changes will likely
> be needed in backporting gixes to the "Shenandoah enabled 11u" mode,
> carefully examination on a case by case basis will be needed, at
> least as
> long as "no shenandoah exists" downstreams of 11u still depend on it
> for updates.

Sorry, I still don't see this as a problem. Careful review of
backports, looking at the gc-shenandoah tag in bug-reports, testing of
builds both with and without Shenandoah, that should cover it?

> > Gil, would the change like this ease your concerns?
> 
> If we could provably show that no code differences exist in builds
> that
> do no choose to include the change, it would be hard for me to argue
> against upstream inclusion based on it having a potential for
> breaking
> a downstream distro's stability, no matte rhow neccesary or not it
> is…
> 
> So yes, the proposed approach could work if we can figure out
> the technical ways to achieve it and "keep it achieved" for some time
> into the future.
> 
> We will likely need to be able to mechanically verify that things
> remain
> like this even as code is introduced in the future. Manual review
> cannot
> achieve that alone probably, so we will likely need some build
> tooling
> to verify this every time.

Another problem with that is this: we can probably mechanically prove
it now, because we have the vanilla 11u build to compare against. Once
Shenandoah is integrated, what would you compare the -shenandoahgc
build against?

In other words: what exactly is it that you want to prove then? I don't
understand it.

Cheers,
Roman