RFR(M): 8000244: G1: Ergonomically set MarkStackSize and use virtual space for global marking stack

[Bengt Rutisson]

Hi John,

Sorry for being late with the feedback on this.

I think it looks good.

A question regarding warnings:

When we are setting up the ConcurrentMark instance we use a lot of 
"warning" in concurrentMark.cpp and return false. This will cause the 
ConcurrentMark instance to not have completed its initiation. This is 
detected in G1CollectedHeap::initialize() and it will call Snippet 
vm_shutdown_during_initialization().

To me it is kind of confusing to have a warning when we can not recover 
from it. I kind of think it would be better to print an error message 
and exit the VM on the spot. I'm not sure what the most common pattern 
is, but I thought warnings were used for things that will allow the VM 
to proceed but in a less than optimal state. But in this case we can not 
proceed at all.

On the other hand I'm kind of surprised that CMMarkStack::expand() does:
  fatal("Not enough swap for expanded marking stack capacity")
when it can not expand. Wouldn't it make sense to just print a warning 
in this case and go on using the previously reserved and committed 
virtual space? We are just using a smaller mark stack size than we would 
like, but we should still be able execute, right?


Very minor things:

I don't think we need "reasonable" default values for MarkStackSizeMax 
in globals.hpp anymore when we always override it in 
Arguments::set_ergonomics_flags(). I would suggest having something like 
"product(uintx, MarkStackSizeMax, 0," instead of "product(uintx, 
MarkStackSizeMax, NOT_LP64(4*M) LP64_ONLY(512*M),".

Line 279 in the new code for concurrentMark.cpp. You added an extra 
space that I think should not be there.

Thanks,
Bengt

On 2012-10-11 19:37, John Cuthbertson wrote:
> Hi Everyone,
>
> I have a new webrev based upon comments and observations from Jon 
> Masamitsu and Peter Kessler which can be found at: 
> http://cr.openjdk.java.net/~johnc/8000244/webrev.1/
>
> Thanks,
>
> JohnC
>
> On 10/02/12 15:45, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> Can I have a couple of volunteers review the change for this CR - the 
>> webrev can be found at: 
>> http://cr.openjdk.java.net/~johnc/8000244/webrev.0/
>>
>> Summary:
>> In response code review comments from Bengt for CR 7188263, I decided 
>> to deal with the marking stack in its own CR - these are those changes.
>>
>> The allocation of G1's global marking stack now matches that of CMS 
>> and is allocated from virtual memory instead of C heap. Further the 
>> size of the marking stack has been changed from a fixed 
>> 128*TASKQUEUE_SIZE (16M entry capacity in a 64 bit JVM), which is the 
>> equivalent of the task queues of 128 threads, to a value based upon 
>> the actual number of parallel marking threads - with a suitable 
>> default minimum (4M entry capacity in a 64 bit JVM). The 4M entry 
>> capacity is the equivalent of the task queues for 32 threads.  I have 
>> also mimicked CMS and added code to expand the marking stack in the 
>> event that concurrent marking is aborted and restarted due to 
>> overflowing the stack. The default maximum was the previous fixed 
>> value (128*TASKQUEUE_SIZE). Hence we go a maximum of 2 expansions 
>> before we have a marking stack sized as it was previously. 
>> Additionally I have rearranged (some of) the ConcurrentMark 
>> initialization code to (hopefully) make it a bit more robust w.r.t. 
>> allocation failures.
>>
>> This CR does not address the allocation of the liveness accounting 
>> data structures - those will be handled as 7188263.
>>
>> Testing:
>> * command line testing with some instrumentation
>> * GC test suite with a low IHOP, heap and marking verification, and 
>> forced marking overflows (and instrumentation)
>> * GC test suite (normal)
>> * jprt
>> * refworkload (I didn't see any overflows in my runs, with an 8Gb 
>> heap, but it could happen).
>>
>> Thanks,
>>
>> JohnC
>