RFR(M): 8000244: G1: Ergonomically set MarkStackSize and use virtual space for global marking stack
bengt.rutisson at oracle.com
Wed Dec 5 08:35:42 UTC 2012
Sorry for the late reply!
On 11/28/12 12:28 AM, John Cuthbertson wrote:
> Hi Bengt,
> Sorry for the delay in getting back to you on this. Replies are
> 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.
I think it is a good idea to push it as a separate change.
> 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.
If we move the code in CMMarkStack::allocate() into the constructor of
CMMarkStack we would not need to add a default consturctor to
ReservedSpace. I kind of think it would be more natural to have the
setup in the constructor anyway.
> 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.
Pushing it as a separate change is definitely a good idea. If
re-committting does not work on a particular platform I guess we are
just back to the old behaviour, right? That a failed expand of the mark
stack will exit the VM.
>> 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().
Yes, you are right. I don't remember how I was thinking when I
>> 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):
Looks good! Ship it!
> Webrev for the just the reuse of the previous reserved space:
I think it would be worth trying to work around having to add the
default constructor to ReserveSpace. But other than that it looks good.
> Combined webrev: http://cr.openjdk.java.net/~johnc/8000244/webrev.all/
More information about the hotspot-gc-dev