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

[John Cuthbertson]

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