Hi Roman,
On 12/3/18 8:27 PM, Roman Kennke wrote:
Round 5 of Shenandoah review includes:
- A fix for the @requires tag in TestFullGCCountTest.java. It should be
correct now. We believe the CMS @requires was also not quite right and
fixed it the same.
It reads now: Don't run this test if:
- Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is
true, as set by harness
- Actual GC set by harness is Shenandoah *and*
ExplicitGCInvokesConcurrent is not set false by harness (it's true by
default in Shenandoah, so this needs to be double-inverteed).
The @requires for CMS was wrong before (we think), because it would also
filter defaultGC + ExplicitGCInvokesConcurrent.
- Sorting of macros was fixed, as was pointed out by Per
- Some stuff was added to SA, as suggested by Jini
- Rebased on most current jdk/jdk code
Webrevs:
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/
I also need reviews from GC reviewers for the CSR:
https://bugs.openjdk.java.net/browse/JDK-8214349
I already got reviews for:
[x] shared-runtime (coleenp)
[x] shared-compiler (kvn)
I got reviews for shared-build, but an earlier version, so maybe makes
sense to look over this again. Erik J, Magnus?
I still need approvals for:
[ ] shared-build (kvn, erikj, ihse, pliden)
[ ] shared-gc (pliden, kbarrett)
[ ] shared-serviceability (jgeorge, pliden)
[ ] shared-tests (lmesnik, pliden)
The above parts look good to me. Reviewed.
Just one tiny nit (and I don't need to see a new webrev for this):
In src/hotspot/share/gc/shared/gcCause.cpp you have this:
+ case _shenandoah_allocation_failure_evac:
+ return "Allocation Failure During Evac";
+
+ case _shenandoah_stop_vm:
+ return "Stopping VM";
+
+ case _shenandoah_concurrent_gc:
+ return "Shenandoah Concurrent GC";
+
+ case _shenandoah_traversal_gc:
+ return "Shenandoah Traversal GC";
+
+ case _shenandoah_upgrade_to_full_gc:
+ return "Shenandoah Upgrade To Full GC";
+
And in
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shared/GCCause.java
you have this:
+ _shenandoah_stop_vm ("Stop VM"),
+ _shenandoah_allocation_failure_evac ("Allocation Failure During
Evacuation"),
+ _shenandoah_concurrent_gc ("Concurrent GC"),
+ _shenandoah_traversal_gc ("Traversal GC"),
+ _shenandoah_upgrade_to_full_gc ("Upgrade to Full GC"),
It would be good to have the exact same strings in both places. There
are currently small differences in all of them. "Evac" vs "Evacuation",
"Stop" vs "Stopping", "Shenandoah" vs "", etc.
May I also suggest that you skip "Shenandoah" in things like "Shenandoah
Concurrent GC" as I kind of think it's implied by the context. But I
also know that CMS/G1 isn't consistent on this point. An alternative
would be to add "Shenandoah" to all of the strings to keep things
consistent, but I'm not sure I like that better. You decide.
[ ] shenandoah-gc
[ ] shenandoah-tests
I haven't looked very much on these parts, and I didn't plan to do so in
detail right now. I think it's fine of the folks that have been working
on the Shenandoah code reviewed this.
cheers,
Per
Thanks for your patience and ongoing support!
Cheers,
Roman
Hi all,
here comes round 4 of Shenandoah upstreaming review:
This includes fixes for the issues that Per brought up:
- Verify and gracefully reject dangerous flags combinations that
disables required barriers
- Revisited @requires filters in tests
- Trim unused code from Shenandoah's SA impl
- Move ShenandoahGCTracer to gc/shenandoah
- Fix ordering of GC names in various files
- Rename UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/
Thanks everybody for taking time to review this!
Roman
Hello all,
Thanks so far for all the reviews and support!
I forgot to update the 'round' yesterday. We are in round 3 now :-)
Also, I fixed the numbering of my webrevs to match the review-round. ;-)
Things we've changed today:
- We moved shenandoah-specific code out of .ad files into our own .ad
files under gc/shenandoah (in shenandoah-gc), how cool is that? This
requires an addition in build machinery though, see
make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build).
- Improved zero-disabling and build-code-simplification as suggested by
Magnus and Per
- Cleaned up some leftovers in C2
- Improved C2 loop opts code by introducing another APIs in
BarrierSetC2. See the new APIs in shared-gc under BarrierSetC2.hpp.
- I don't see where it makes sense to put INCLUDE_SHENANDOAHGC guards now.
- We would all very much prefer to keep ShenandoahXYZNode names, as
noted earlier. This stuff is Shenandoah-specific, so let's just call it
that.
- Rehashed Shenandoah tests (formatting, naming, directory layout, etc)
- Rebased on jdk-12+22
- Question: let us know if you need separate RFE for the new BSC2 APIs?
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/
Thanks,
Roman
Alright, we fixed:
- The minor issues that Kim reported in shared-gc
- A lot of fixes in shared-tests according to Leonid's review
- Disabled SA heapdumping similar to ZGC as Per suggested
Some notes:
Leonid: test/hotspot/jtreg/gc/TestFullGCCount.java was actually
correct. The @requires there means to exclude runs with both CMS and
ExplicitGCInvokesConcurrent at the same time, because that would be
(expectedly) failing. It can run CMS, default GC and any other GC just
fine. Adding the same clause for Shenandoah means the same, and filters
the combination (+UseShenandoahGC)+(+ExplicitGCInvokesConcurrent). I
made the condition a bit clearer by avoiding triple-negation.
See:
http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008457.html
Per: Disabling the SA part for heapdumping makes 2 tests fail:
- test/hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java
- test/hotspot/jtreg/serviceability/sa/TestHeapDumpForLargeArray.java
we filter them for Shenandoah now. I'm wondering: how do you get past
those with ZGC?
See:
http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008466.html
(Note to Leonid and tests reviewers: I'll add those related filters in
next round).
Vladimir: Roland integrated a bunch of changes to make loop* code look
better. I can tell that we're not done with C2 yet. Can you look over
the code and see what is ok, and especially what is not ok, so that we
can focus our efforts on the relevant parts?
Updated set of webrevs:
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/01/
Thanks,
Roman
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
