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

[John Cuthbertson]

Hi Bengt,

Sorry for the delay in getting back to you on this. Replies are 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.

As I mentioned in a previous reply - the change to use warnings and an 
error status was prompted by comments by Jon Masa. I don't think there 
is any consensus in the current hotspot sources. IIRC the coding 
guidelines class that we all recently went through discouraged the use 
of multiple error exits.

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

I have the code changes that re-uses the previous unexpanded source but 
I would rather do it as a separate CR (I'll include the webrev). The 
current code uses the CMS mark stack expansion code as a base.

To reuse the previous space we need to cache the ReservedSpace in the 
CMMarkStack instance to re-initialize the _virtual_space. As a result we 
need to provide a default constructor for ReservedSpace. Another concern 
is that I'm not sure that re-committing the original unexpanded space 
won't cause a problem on some platforms. I've tested the code with 
forced re-commits and it seems to work but would rather it was pushed as 
a separate change.
>
>
> 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),".

I can't find the code that sets the flag in 
Arguments::set_ergonomics_flags() and it is only referenced (not 
changed) in Arguments::set_cms_and_parNew_flags(). The only override I 
see is in Arguments::set_g1_flags().
>
> 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. Done.

New webrev (minus the code that reuses the previous reserved space): 
http://cr.openjdk.java.net/~johnc/8000244/webrev.2/

Webrev for the just the reuse of the previous reserved space: 
http://cr.openjdk.java.net/~johnc/8000244/webrev.reuse-old-marking-stack/

Combined webrev: http://cr.openjdk.java.net/~johnc/8000244/webrev.all/

Thanks,

JohnC