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

[Calvin Cheung]

Hi David,

Thanks for your review.

On 4/22/16, 12:45 AM, David Holmes wrote:
> 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.
How about the following?

// skip the following block of code for 32-bit platforms
#if (defined(COMPILER2) && defined(_LP64)) || INCLUDE_JVMCI
   // For 64-bit compiler2 (-server), keep the default sharing off unless
   // it is enabled via -Xshare:on.
   if (!DumpSharedSpaces && !RequireSharedSpaces &&
       (FLAG_IS_DEFAULT(UseSharedSpaces) || !UseSharedSpaces)) {
     no_shared_spaces("COMPILER2 default: -Xshare:auto | off, have to 
manually setup to on.");
   }
#endif
>
> ---
>
> Query: In the tests, should they be @driver rather than @run ?
I think you meant the following?
@run driver <class>

 From the jtreg documentation:
> driver[/fail][/timeout=<seconds>] <class> <arg>*
> Invoke the main method of the specified class, passing any arguments 
> after the class name. Although superficially similar to @run main, 
> this is for use when it is desirable to perform additional setup or 
> configuration before running the class containing the actual test 
> code, possibly in a child VM.
>
> The named <class> will be compiled on demand, just as though an "@run 
> build <class>" action had been inserted before this action. If this 
> action requires classes other than <class> to be up to date, insert an 
> appropriate build action before this action.

For the 2 tests in this change, there's no setup or configuration step 
required.
So I think it is unnecessary to use "driver" action.

I did try the 2 tests with the "driver" action and they worked as well.
e.g. @run driver DefaultSharing

Let me know if you want to see a new webrev with the changes above.

thanks,
Calvin

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