[aarch64-port-dev ] RFR: Shenandoah import

[Andrew Haley]

On 02/10/17 13:29, Roman Kennke wrote:
> This imports Shenandoah changes from shenandoah/jdk8u/hotspot into the 
> integration repository aarch64-port/jdk8u-shenandoah/hotspot .
> 
> It includes (and thus supercedes) changes proposed here:
> 
> http://mail.openjdk.java.net/pipermail/aarch64-port-dev/2017-August/004854.html
> 
> Instead of doing bulk-import I went back to transplanting 
> single-changeset import. I can avoid the merge conflicts simply by not 
> merging back from aarch64-port/jdk8u-shenandoah but instead from 
> aarch64-port/jdk8u. Plus we get the benefit of preserving the original 
> changesets, make bisecting more useful, etc.
> 
> It also includes newer backports from Aug 21 - today.
> 
> Everything has been extensively tested and baked in Shenandoah land.
> 
> Notably, it includes a bunch of changes done by Roland that better 
> isolates Shenandoah specific code paths in C2 (thus minimizing 
> Shenandoah exposure in non-Shenandoah runs).
> 
> Besides that, all of it is bugfixing plus performance and usability 
> improvements.
> 
> The complete list of changesets is here:
> 
> http://cr.openjdk.java.net/~rkennke/jdk8u-shenandoah-integration-2017-10-02/changesets.txt 
> <http://cr.openjdk.java.net/%7Erkennke/jdk8u-shenandoah-integration-2017-10-02/changesets.txt>
> 
> The total webrev is here:
> 
> http://cr.openjdk.java.net/~rkennke/jdk8u-shenandoah-integration-2017-10-02/webrev.00/ 
> <http://cr.openjdk.java.net/%7Erkennke/jdk8u-shenandoah-integration-2017-10-02/webrev.00/>
> 
> Please review! Thanks, Roman

It looks basically sane, but some caveats:

   I haven't looked much at the Shenandoah-specific code.  I assume
   that's been done at the Shenandoah dev stage already.

   I also haven't looked at test code.

   The code reorganization makes it very hard to review some of this
   patch.  I know that it's necessary from time to time to rename and
   reorganize things, but it's important to make sure that such
   changesets don't (deliberately, anyway) also contain semantic
   changes.

My main concern is to ensure that Shenandoah doesn't break anything
else.  I wish there were some easy way to separate shared from
Shenandoah code in a webrev, but never mind.

Some minor stuff:

--- a/src/share/vm/opto/macro.cpp
+++ b/src/share/vm/opto/macro.cpp
@@ -624,7 +624,7 @@
                                    k < kmax && can_eliminate; k++) {
           Node* n = use->fast_out(k);
           if (!n->is_Store() && n->Opcode() != Op_CastP2X &&
-              !n->is_g1_wb_pre_call()) {
+              (!UseShenandoahGC || !n->is_g1_wb_pre_call())) {

This logic is super-confusing.  It could be

           if (! UseShenandoahGC &&
               !n->is_Store() && n->Opcode() != Op_CastP2X &&
               !n->is_g1_wb_pre_call()) {

it could even be

           if (! UseShenandoahGC &&
               ! (n->is_Store() || n->Opcode() == Op_CastP2X ||
                  n->is_g1_wb_pre_call()) {

but I guess that'd be a divergence from upstream, which we want to
minimize.  in general, though, for things that we add we should reduce
the complexity of boolean operations where it makes sense.

In general, please put UseShenandoahGC guards at the start of boolean
expressions wherever possible, as a flag to the reader.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671