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

[John Cuthbertson]

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
>