Hi Leonid,
I looked at the tests changes only. It seems that a some tests might
start failing if JDK built without Shenandoah GC. Also some test are not
going to be selected as expected.
Unfortunately logic of '@requires' and @run in jtreg tests doesn't
always work well for specific cases of different GC selection. The
'@requires' tags are combined with '&' and whole test is selected or
not. Test always execute ALL @run actions so it fails if option
-XX:+UseShenandoahGC is not supported (not valid). The only way to split
'run' actions is to add more @test with same sources. They could be in
the same file.
See detailed info about jtreg tags
here:http://openjdk.java.net/jtreg/tag-spec.html
<http://openjdk.java.net/jtreg/tag-spec.html>
Thanks for pointing this out.
I fixed all of what you pointed out (I think) and some more:
http://cr.openjdk.java.net/~rkennke/fix-shared-tests/webrev.02/
It will show up in round2 of this review.
Some more comments inline:
Could you please run your tests with with JDK which built without
Shenandoah GC. It helps to verify that Shenandoah-specific tests/runs
are not selected when this GC is not supported.
I did now, and they are good (with above changes).
Also it would be nice
to verify that there are no any valid tests which became filtered out
with your patch. The running of all test suite with available but not
selected Shenandoah GC and enabled Graal also might help to verify
@requires settings. (It doesn't help when Shenandoah GCis not supported
though.)
I'll see into running more combinations. So far I did hotspot_gc and a
few others with and without Shenandoah.
I haven't looked at the tests in directory gc/shenandoah in details. But
all of them should be guarded with @requires. Placing them in separate
directory is not enough. See G1 tests as example:
http://hg.openjdk.java.net/jdk/jdk/file/10c6e9066819/test/hotspot/jtreg/gc/g1/TestEagerReclaimHumongousRegions.java
<http://hg.openjdk.java.net/jdk/jdk/file/10c6e9066819/test/hotspot/jtreg/gc/g1/TestStringDeduplicationFullGC.java#l29>
Right. We've done that now.
See more detailed comments about shared tests:
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/TEST.groups.sdiff.html
219: tier2_gc_shenandoah = \
Usually tierN doesn't include tests from tierN-1. TierN is executed
after TierN-1 completed so no need to re-run the same tests. The
typical groups might looks like:
tier1_gc_shenandoah = \
gc/shenandoah/<tier1-tests> \
other-tests
tier2_gc_shenandoah = \
gc/shenandoah/<tier2-tests>\
-:tier1_gc_shenandoah
tier3_gc_shenandoah = \
gc/shenandoah/ \ //all-other-tests
-:tier2_gc_shenandoah
We fixed that.
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/CriticalNativeArgs.java.html
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/stress/CriticalNativeStress.java.html
So your test will be skipped if any of -XX:+UseEpsilonGC or
-XX:+UseShenandoahGC is set. Also test might run only all run actions or
none of them. It would be better to split this test into 2 tests. So
epsilon tests might be executed if Shenandoah is absent.
I think that (vm.bits == "64") is redundant in tag (you set x64 or arm64).
Please use 4-space indentation in java code.
All fixed.
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/TestFullGCCount.java.sdiff.html
Even original requires seems confusing to me (but it works for CMS/G1 pair)
28 * @requires !(vm.gc.ConcMarkSweep &
vm.opt.ExplicitGCInvokesConcurrent == true)
So currently test is executed if GC is CMS or default GC and
ExplicitGCInvokesConcurrent is not set to true.
With your additional requirements 'vm.gc == "Shenandoah"' test is not
selected if ANY GC is set. Test doesn't set any GC itself so only
default GC might be tested. See [1].
I split them and fixed the @requires to run with Shenandoah but only
with -ExplicitGCInvokesConcurrent.
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/TestHumongousReferenceObject.java.sdiff.html
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/TestSystemGC.java.sdiff.html
Tests will always just fail if -XX:+UseShenandoahGC is not supported.
(invalid option) You might want to split test is it done for CMS GC.
Done.
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/arguments/TestAlignmentToUseLargePages.java.sdiff.html
I think
56 * @requires vm.gc=="null" & !vm.graal.enabled
should be something like @requires vm.gc.Shenandoah & !vm.graal.enabled
Yes. Fixed them.
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/arguments/TestUseCompressedOopsErgo.java.sdiff.html
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/class_unloading/TestClassUnloadingDisabled.java.sdiff.html
The same for 62 * @requires vm.gc=="null" & !vm.graal.enabled
and
72 * @requires vm.gc=="null" & !vm.graal.enabled
Fixed them too.
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/ergonomics/TestDynamicNumberOfGCThreads.java.sdiff.html
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/logging/TestGCId.java.sdiff.html
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/metaspace/TestMetaspacePerfCounters.java.sdiff.html
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/runtime/CompressedOops/UseCompressedOops.java.sdiff.html
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/runtime/MemberName/MemberNameLeak.java.sdiff.html
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/metaspace/TestMetaspacePerfCounters.java.sdiff.html
Also these tests are going to be run with all GC and fails if Shenandoah
is not supported.
Right. I fixed those and a few others I have found. Those which drive
Shenandoah at runtime now have a runtime check
(GC.Shenandoah.isSupported() which uses WB). Others have split test
sections.
I'll upload round 2 of review changesets, which contains the fixes in a bit.
Thanks a *LOT* for detailed review.
Roman
Leonid
[1] http://openjdk.java.net/jtreg/tag-spec.html
On 11/26/18 1:39 PM, 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
