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

[Bengt Rutisson]

Hi John,

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

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 
suggested this.

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

Looks good! Ship it!

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

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.

Thanks,
Bengt

>
>
> Combined webrev: http://cr.openjdk.java.net/~johnc/8000244/webrev.all/
>
> Thanks,
>
> JohnC