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

[Jiangli Zhou]

Hi Calvin,

Please see comments below.

> On Apr 22, 2016, at 2:18 PM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
> 
> 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

I felt the same as David when looking at the code. The above simplified comment looks better. However the comments only explain the selection logic for 64-bit server. It would be helpful to find out why -Xshare:auto is off by default with INCLUDE_JVMCI enabled as part of your exercise.

Thanks,

Jiangli

>> 
>> ---
>> 
>> 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