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

[Bengt Rutisson]

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