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

[John Cuthbertson]

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.

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