RFR(M): 8000244: G1: Ergonomically set MarkStackSize and use virtual space for global marking stack
john.cuthbertson at oracle.com
Tue Nov 27 23:28:29 UTC 2012
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
> 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):
Webrev for the just the reuse of the previous reserved space:
Combined webrev: http://cr.openjdk.java.net/~johnc/8000244/webrev.all/
More information about the hotspot-gc-dev