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

[John Cuthbertson]

Hi Bengt,

Thanks for the review. Response inline....

On 10/24/12 09:29, Bengt Rutisson wrote:
>
> 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.

Yeah. I originally had it the way you suggest but changed it as a result 
of feedback from Jon. I'm not sure there is a common pattern in hotspot 
for this. I think CMS uses warnings and checks a flag and I just 
extended the idea to G1's concurrent marking. I'm not sure what's the 
better approach here.

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

When looking at the original CMS code I thought the same thing. I think, 
however, the problem is that we have already released the space:

  // Double capacity if possible
  size_t new_capacity = MIN2(_capacity*2, MarkStackSizeMax);
  // Do not give up existing stack until we have managed to
  // get the double capacity that we desired.
  ReservedSpace rs(ReservedSpace::allocation_align_size_up(
                   new_capacity * sizeof(oop)));
  if (rs.is_reserved()) {
    // Release the backing store associated with old stack
    _virtual_space.release();
    // Reinitialize virtual space for new stack
    if (!_virtual_space.initialize(rs, rs.size())) {
      fatal("Not enough swap for expanded marking stack");
    }

but it looks like VirtualSpace::release() doesn't actually release the 
memory. I wonder if that was always the case. Anyway I think your 
suggestion is a good one. I'll see if it will work - we should be able 
to get the boundaries of the previous committed space before we call 
release().

> 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),".
If this is true then I agree. I'll check it out.

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

Thanks for catching that.

I should have a new webrev with the changed expansion code and no 
default for MarkStackSizeMax in the next day or so.

Thanks,

JohnC