RFR(S): 7192128: G1: Extend fix for 6948537 to G1's BOT

[Bengt Rutisson]

Hi John,

On 2012-08-21 18:50, John Cuthbertson wrote:
> Hi Bengt,
>
> Thanks again for the review. I spoke to Vladimir about why we call 
> VM_Version::initialize() twice on sparc. The main reason is because 
> SPARC does not have an easy way to get the CPUID and so 
> VM_Version::initialize is called before argument processing  to 
> initialize CPU flags like "is_niagara()" etc.

Thanks for investigating this further. I still think it would be better 
to split VM_Version::initialize() up into two methods on Sparc or at 
least have a guard if it has already been initialized. But that is 
unrelated to your change.

Bengt


>
> JohnC
>
> On 08/20/12 01:20, Bengt Rutisson wrote:
>>
>> Hi John,
>>
>> "VM_Version_init gets called twice on sparc."
>>
>> I didn't dig any deeper into that, but it does sound like something 
>> that should be cleaned up. But not as part of your changes.
>>
>> Going back to your original code is fine. I'm also fine with leaving 
>> the other duplicated code until we remove the G1 BOT implementation.
>>
>> So, again: Ship it! :)
>> Bengt
>>
>> On 2012-08-17 23:10, John Cuthbertson wrote:
>>> Hi Bengt,
>>>
>>> I believe I'll have to leave the warnings being emitted from 
>>> g1CollectedHeap.cpp and concurrentMarkSweepGeneration.cpp. Moving 
>>> the warning into VM_Version::initialize() causes the warning to come 
>>> out twice:
>>>
>>> dr-evil{jcuthber}:293> ./gamma -XX:+UnlockExperimentalVMOptions 
>>> -XX:+UseG1GC -XX:+UseMemSetInBOT -version
>>> Using java runtime at: 
>>> /net/jre.us.oracle.com/p/v42/jdk/7/fcs/b147/binaries/solaris-sparcv9/jre 
>>>
>>> Java HotSpot(TM) 64-Bit Server VM warning: Experimental flag 
>>> -XX:+UseMemSetInBOT is known to cause instability on sun4v; please 
>>> understand that you are using at your own risk!
>>> Java HotSpot(TM) 64-Bit Server VM warning: Experimental flag 
>>> -XX:+UseMemSetInBOT is known to cause instability on sun4v; please 
>>> understand that you are using at your own risk!
>>> java version "1.7.0"
>>> Java(TM) SE Runtime Environment (build 1.7.0-b147)
>>> Java HotSpot(TM) 64-Bit Server VM (build 24.0-b20-internal-jvmg, 
>>> mixed mode)
>>>
>>> It appears we are calling VM_Version::initialize() twice (for 
>>> whatever reason). Once from Arguments::check_vm_args_consistency():
>>>
>>> // Check the consistency of vm_init_args
>>> bool Arguments::check_vm_args_consistency() {
>>>  // Method for adding checks for flag consistency.
>>>  // The intent is to warn the user of all possible conflicts,
>>>  // before returning an error.
>>>  // Note: Needs platform-dependent factoring.
>>>  bool status = true;
>>>
>>> #if ( (defined(COMPILER2) && defined(SPARC)))
>>>  // NOTE: The call to VM_Version_init depends on the fact that 
>>> VM_Version_init
>>>  // on sparc doesn't require generation of a stub as is the case on, 
>>> e.g.,
>>>  // x86.  Normally, VM_Version_init must be called from init_globals in
>>>  // init.cpp, which is called by the initial java thread *after* 
>>> arguments
>>>  // have been parsed.  VM_Version_init gets called twice on sparc.
>>>  extern void VM_Version_init();
>>>  VM_Version_init();
>>>  if (!VM_Version::has_v9()) {
>>>    jio_fprintf(defaultStream::error_stream(),
>>>                "V8 Machine detected, Server requires V9\n");
>>>    status = false;
>>>  }
>>> #endif /* COMPILER2 && SPARC */
>>>
>>> So I'll go back to the original code.
>>>
>>> JohnC
>>>
>>> On 08/17/12 12:02, John Cuthbertson wrote:
>>>> Hi Bengt,
>>>>
>>>> Thanks for reviewing the code so quickly.
>>>>
>>>>
>>>> On 08/16/12 23:18, Bengt Rutisson wrote:
>>>>>
>>>>> Hi John,
>>>>>
>>>>> Great that you found this issue and fixed it so quickly!
>>>>>
>>>>> I think the change looks good.
>>>>>
>>>>> An optional change that you can do if you would like to is:
>>>>>
>>>>> The G1CollectedHeap constructor now has this code:
>>>>>
>>>>> #ifdef SPARC
>>>>>   // Issue a stern warning, but allow use for experimentation and 
>>>>> debugging.
>>>>>   if (VM_Version::is_sun4v() && UseMemSetInBOT) {
>>>>>     assert(!FLAG_IS_DEFAULT(UseMemSetInBOT), "Error");
>>>>>     warning("Experimental flag -XX:+UseMemSetInBOT is known to 
>>>>> cause instability"
>>>>>             " on sun4v; please understand that you are using at 
>>>>> your own risk!");
>>>>>   }
>>>>> #endif
>>>>>
>>>>> Which is duplicated code from the CMSCollector constructor. Could 
>>>>> we move it to a common place? Maybe even include it in 
>>>>> vm_version_sparc.cpp -> VM_Version::initialize() where we have the 
>>>>> other special treatment of UseMemSetInBOT for CMS and G1? 
>>>>> Something like:
>>>>>
>>>>>   if (is_niagara()) {
>>>>>     ...
>>>>>     if (UseMemSetInBOT && (UseConcMarkSweepGC || UseG1GC)) {
>>>>>       if (FLAG_IS_DEFAULT(UseMemSetInBOT) {
>>>>>         FLAG_SET_DEFAULT(UseMemSetInBOT, false);
>>>>>       } else {
>>>>>         warning("Experimental flag -XX:+UseMemSetInBOT is known to 
>>>>> cause instability"
>>>>>                 " on sun4v; please understand that you are using 
>>>>> at your own risk!");
>>>>>       }
>>>>>     }
>>>>>     ...
>>>>>   }
>>>>>
>>>>
>>>> This is a great idea. Done.
>>>>
>>>>> It would also have been nice to be able have a wrapper method for 
>>>>> the 4 places where we now have this type of code:
>>>>>
>>>>>     if (UseMemSetInBOT) {
>>>>>       memset(&_offset_array[index_for(left)], offset, num_cards);
>>>>>     } else {
>>>>>       size_t i = index_for(left);
>>>>>       const size_t end = i + num_cards;
>>>>>       for (; i < end; i++) {
>>>>>         _offset_array[i] = offset;
>>>>>       }
>>>>>     }
>>>>>
>>>>> But I'm fine with leaving it as it is for now.
>>>>>
>>>>>
>>>> I would prefer to leave it as it is for now. We do have a CR to 
>>>> reconcile G1's BOT with the that of the other collectors - I think 
>>>> that is when we can introduce the wrapper (and it would be back 
>>>> down to only two instances). I'll add a comment to that CR.
>>>>
>>>> JohnC
>>>>
>>>
>>
>