RFR(xs): 8152356: Change to -Xshare:auto for 32-bit platforms

[David Holmes]

Hi Calvin,

On 22/04/2016 4:57 PM, Calvin Cheung wrote:
>
> Please review this small fix for enabling -Xshare:auto as the default
> for 32-bit platforms.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8152356
>
> webrev: http://cr.openjdk.java.net/~ccheung/8152356/webrev.00/

I find the selection logic for this to be almost unfathomable - and made 
worse by the conditional compilation! But looking at your changes:

-#if defined(COMPILER2) || INCLUDE_JVMCI || defined(_LP64) || !INCLUDE_CDS
+#if INCLUDE_JVMCI || defined(_LP64) || !INCLUDE_CDS

Ok - 64-bit implies compiler2 as we don't do 64-bit client, so this 
reduction is fine.

-#if defined(COMPILER2) || INCLUDE_JVMCI
+#if (defined(COMPILER2) && defined(_LP64)) || INCLUDE_JVMCI

Ok - we're relaxing the constraint so that we skip this code block for 
32-bit compiler2.

    // Shared spaces work fine with other GCs but causes bytecode rewriting
    // to be disabled, which hurts interpreter performance and decreases
-  // server performance.  When -server is specified, keep the default off
-  // unless it is asked for.  Future work: either add bytecode rewriting
-  // at link time, or rewrite bytecodes in non-shared methods.
+  // server performance. After JDK-8074345, bytecode rewriting is 
enabled for
+  // non-shared classes. So share spaces should no longer affect the 
performance of
+  // benchmarks executed by C2. Enabling the default (-Xshare:auto) on 
32-bit platforms when
+  // -server is specified. When -server is specified on a 64-bit 
platform, keep the default
+  // off unless it is asked for.

I'm not a fan of history lessons in comments - they tend to only make 
sense at the time of writing. If the code works fine now then it works 
fine now, we don't need to document that it didn't used to work fine but 
bug XXX fixed it. I'd simplify the comment to just state what exactly 
you are checking for, because to be honest I find this:

if (!DumpSharedSpaces && !RequireSharedSpaces &&
        (FLAG_IS_DEFAULT(UseSharedSpaces) || !UseSharedSpaces)) {

very hard to unwravel. The first part is fine - not dumping. The second 
part indicates we are either in -Xshare:off or -Xshare:auto. But the 
third part is quite puzzling - and even when I think I have it figured 
out I can't reconcile with what I see when I use -XX:+PrintFlagsFinal. 
So the "what are we doing" is much more important to comment than the 
history.

---

Query: In the tests, should they be @driver rather than @run ?

Thanks,
David

> Testing:
>      JPRT
>      jtreg tests under hotspot/test/runtime
>
> thanks,
> Calvin