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

[Calvin Cheung]

Hi,

I'm withdrawing this RFR as more investigations are required before 
enabling -Xshare:auto for all platforms.

thanks,
Calvin

On 4/22/16, 5:18 PM, Jiangli Zhou wrote:
> Christian, thank you for the information. It seems like INCLUD_JVMCI should be handled the same as COMPILER2 in this case.
>
> I agree with Vladimir’s view and prefer avoiding unnecessary complication.
>
> Thanks,
> Jiangli
>
>> On Apr 22, 2016, at 4:17 PM, Vladimir Kozlov<vladimir.kozlov at oracle.com>  wrote:
>>
>>>> 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.
>>> There is no technical reason; INCLUDE_JVMCI was pretty much added everywhere where there is a #ifdef COMPILER2.
>>>
>> Note, the only one current implementation of JVMCI compiler, Graal, is available only for 64-bit VM. I don't think it will change in a future.
>>
>> I would more expect that we will stop supporting 32-bit Server VM on all platforms (may be except embedded) in a very near future. The only 32-bit platforms with Server VM are Win7, Win8 and Ubuntu. Win10 has only 64-bit VM.
>>
>> Based on this I question the need for this change only for 32-bit VM. I would prefer to see it for all configurations. It will simplify #if conditions you complain about.
>>
>> Note, changing from Client VM to Server VM will have much high impact on user experience on 32-bit platforms than using or not using CDS.
>>
>> David said: "Just because -Xshare:auto is the default, it doesn't mean we have to generate an archive as part of the install process. That is an independent action/decision. This RFE is simply asking the question "if there exists an archive should we use it by default?""
>>
>> If we do 64-bit too it will not change default behavior for 64-bit VM since by default there will be no CDS archive installed. And checking if archive exists is very cheap. If needed we can say in release document that we support it only on 32-bit VM in jdk 9 (as before).
>>
>> And lastly I agree with complains that current condition when it is enabled it very complicated. We should just enable it by default and done with it.
>>
>> Thanks,
>> Vladimir
>>
>> On 4/22/16 3:16 PM, Christian Thalinger wrote:
>>>> On Apr 22, 2016, at 12:08 PM, Jiangli Zhou<jiangli.zhou at oracle.com>  wrote:
>>>>
>>>> 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.
>>> There is no technical reason; INCLUDE_JVMCI was pretty much added everywhere where there is a #ifdef COMPILER2.
>>>
>>>> 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