On 11/27/18 1:37 AM, Roman Kennke wrote:
Hi Vladimir,
You need to check if shenandoahgc is disabled first - check
DISABLED_JVM_FEATURES list (see code for jvmci).
Why? If Shenandoah is disabled, it will be on this list, and therefore
not be built (see JvmFeatures.gmk).
Did you test with --with-jvm-variants=-shenandoahgc to make sure it is disabled?
May be I needed explicitly check jvmci because I need it early to check it for enable Graal build. So it is different
from your case.
Do you support 32-bit x86? You have OPENJDK_TARGET_CPU == xx86 check.
Do you support all x86 OSs?
We can build with 32bit, but it will not run. It seems indeed curious to
enable it. It's probably only there to allow 32bit builds with
Shenandoah enabled, just to verify that we have all the relevant LP64
checks in code in place. I will check it with my collegues.
What about OS? Do you support Windows, MacOS? I did not see any OS specific
changes. So may be it is alright.
Why you set VM CFLAGS when shenandoahgc is not enabled? It is in
JvmFeatures.gmk.
This *disables* and excludes Shenandoah if not enabled.
+ifneq ($(call check-jvm-feature, shenandoahgc), true)
+ JVM_CFLAGS_FEATURES += -DINCLUDE_SHENANDOAHGC=0
+ JVM_EXCLUDE_PATTERNS += gc/shenandoah
pretty much the same pattern as zgc and other features.
and then we add some flags when Shenandoah is enabled:
+else
+ JVM_CFLAGS_FEATURES += -DSUPPORT_BARRIER_ON_PRIMITIVES
-DSUPPORT_NOT_TO_SPACE_INVARIANT
+endif
... which are required for building with Shenandoah enabled.
My bad. Somehow I thought it was reverse. Too much coffee at the morning. :(
Looks good.
I looked on C2 changes. It has INCLUDE_SHENANDOAHGC, checks and new gc
specific nodes. This looks intrusive. I hope you clean it up.
There are a few places with INCLUDE_SHENANDOAHGC checks where it seemed
excessive to add a whole API just for a tiny, very GC specific check. We
still do have intrusive changes in loop*, which we are working on to
resolve. We declare+define all our GC specific nodes in
shenandoahBarrierSetC2.hpp and related Shenandoah-specific files. The
only things in shared code is the usual declarations in classes.hpp/cpp
and node.hpp to get is_ShenandoahBarrier() an similar queries. This
seems in-line with what other GCs do (look for e.g. LoadBarrier). Please
be specific which changes in opto you'd like to see resolved (and don't
look at loop* source files at this point ;-) ).
Is it possible to reuse LoadBarrier by adding GC specific flag to it?
I am not against adding new nodes if really needed. My concern is about using
GC name in node's names.
Yes, I am fine with very few INCLUDE_SHENANDOAHGC.
Thanks,
Vladimir
Thanks for reviewing!
Roman
Thanks,
Vladimir
On 11/26/18 2:47 PM, Erik Joelsson wrote:
Build changes look ok to me.
/Erik
On 2018-11-26 13:39, Roman Kennke wrote:
Hi,
This is the first round of changes for including Shenandoah GC into
mainline.
I divided the review into parts that roughly correspond to the
mailing lists
that would normally review it, and I divided it into 'shared' code
changes and
'shenandoah' code changes (actually, mostly additions). The intend is to
eventually
push them as single 'combined' changeset, once reviewed.
JEP:
https://openjdk.java.net/jeps/189
Bug entry:
https://bugs.openjdk.java.net/browse/JDK-8214259
Webrevs:
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/
For those who want to see the full change, have a look at the
shenandoah-complete
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shenandoah-complete/>
directory,
it contains the full combined webrev. Alternatively, there is the file
shenandoah-master.patch
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shenandoah-master.patch>,
which is what I intend to commit (and which should be equivalent to the
'shenandoah-complete' webrev).
Sections to review (at this point) are the following:
*) shenandoah-gc
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shenandoah-gc/>
- Actual Shenandoah implementation, almost completely residing in
gc/shenandoah
*) shared-gc
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-gc/>
- This is mostly boilerplate that is common to any GC
- referenceProcessor.cpp has a little change to make one assert not
fail (next to CMS and G1)
- taskqueue.hpp has some small adjustments to enable subclassing
*) shared-serviceability
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-serviceability/>
- The usual code to support another GC
*) shared-runtime
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-runtime/>
- A number of friends declarations to allow Shenandoah iterators to
hook up with,
e.g. ClassLoaderData, CodeCache, etc
- Warning and disabling JFR LeakProfiler
- fieldDescriptor.hpp added is_stable() accessor, for use in
Shenandoah C2 optimizations
- Locks initialization in mutexLocker.cpp as usual
- VM operations defines for Shenandoah's VM ops
- globalDefinitions.hpp added UINT64_FORMAT_HEX_W for use in
Shenandoah's logging
- The usual macros in macro.hpp
*) shared-build
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-build/>
- Add shenandoah feature, enabled by default, as agreed with
Vladimir K. beforehand
- Some flags for shenandoah-enabled compilation to get
SUPPORT_BARRIER_ON_PRIMITIVES
and SUPPORT_NOT_TO_SPACE_INVARIANT which is required for
Shenandoah's barriers
- --param inline-unit-growth=1000 settings for 2 shenandoah source
files, which is
useful to get the whole marking loop inlined (observed
significant
regression if we
don't)
*) shared-tests
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/>
- Test infrastructure to support Shenandoah
- Shenandoah test groups
- Exclude Shenandoah in various tests that can be run with
selected GC
- Enable/add configure for Shenandoah for tests that make sense to
run with it
*) shenandoah-tests
<http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shenandoah-tests/>
- Shenandoah specific tests, most reside in gc/shenandoah
subdirectory
- A couple of tests configurations have been added, e.g.
TestGCBasherWithShenandoah.java
I intentionally left out shared-compiler for now, because we have some
work left to do
there, but if you click around you'll find the patch anyway, in case you
want to take
a peek at it.
We have regular builds on:
- {Linux} x {x86_64, x86_32, armhf, aarch64, ppc64el, s390x}
- {Windows} x {x86_64},
- {MacOS X} x {x86_64}
This also routinely passes:
- the new Shenandoah tests
- jcstress with/without aggressive Shenandoah verification
- specjvm2008 with/without aggressive Shenandoah verification
I'd like to thank my collegues at Red Hat: Christine Flood, she deserves
the credit for being the original inventor of Shenandoah, Aleksey
Shiplëv, Roland Westrelin & Zhengyu Gu for their countless
contributions, everybody else in Red Hat's OpenJDK team for testing,
advice and support, my collegues in Oracle's GC, runtime and compiler
teams for tirelessly helping with and reviewing all the GC interface and
related changes, and of course the many early adopters for reporting
bugs and success stories and feature requests: we wouldn't be here
without any of you!
Best regards,
Roman
