RFR (XS): 8021879: G1: G1HeapRegionSize flag value not updated correctly

[Bengt Rutisson]

Hi Thomas,

Thanks for the review!

Good idea to add a regression test. I added a test, here is an updated 
webrev (only the test was added, the code change is the same):

http://cr.openjdk.java.net/~brutisso/8021879/webrev.01/

This is again a test that will only work with G1 and will fail if other 
GCs are specified but the test harness. I will contact SQE about this 
issue. We have several such tests by now, so I think we should go ahead 
and push this test as it is and work with SQE to figure out how to 
handle all of those tests.

Thanks,
Bengt


On 7/30/13 2:39 PM, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2013-07-30 at 14:11 +0200, Bengt Rutisson wrote:
>> Hi everyone,
>>
>> Could I have a couple of reviews for this very small change?
>>
>> http://cr.openjdk.java.net/~brutisso/8021879/webrev.00/
>>
>> Background:
>>
>> The G1HeapRegionSize flag is not updated with the actual value of the
>> region size. This makes it impossible to figure out the heap region
>> size using PrintFlagsFinal:
>>
>> $ java -XX:+UseG1GC -XX:+PrintFlagsFinal -version | grep
>> G1HeapRegionSize
>>      uintx G1HeapRegionSize = 0 {product}
>>
>> Or when setting it to the wrong value (this will actually use a region
>> size of 8m):
>>
>> $ java -XX:+UseG1GC -XX:G1HeapRegionSize=11m -XX:+PrintFlagsFinal
>> -version | grep G1HeapRegionSize
>>      uintx G1HeapRegionSize := 11534336 {product}
>>
>>
>> With the proposed patch I get this output:
>>
>> $ java -XX:+UseG1GC -XX:+PrintFlagsFinal -version | grep
>> G1HeapRegionSize
>>      uintx G1HeapRegionSize                         := 1048576
>> {product}
>>
>> and:
>>
>> $ java -XX:+UseG1GC -XX:G1HeapRegionSize=11m -XX:+PrintFlagsFinal
>> -version | grep G1HeapRegionSize
>>      uintx G1HeapRegionSize                         := 8388608
>> {product}
> Looks good. What about a regression test that verifies that this value
> is set to something reasonable after startup?
>
> There are already some helper classes to parse PrintFlagsFinal output in
> the gc/g1 test directory.
>> Here are also some interesting scenarios that are visible with my
>> suggested patch:
>>
>> $ java -XX:+UseG1GC -Xmx64g  -XX:+PrintFlagsFinal -version | grep
>> G1HeapRegionSize
>>      uintx G1HeapRegionSize                         := 1048576
>> {product}
>>
>> $ java -XX:+UseG1GC -Xms64g -Xmx64g  -XX:+PrintFlagsFinal -version |
>> grep G1HeapRegionSize
>>      uintx G1HeapRegionSize                         := 33554432
>> {product}
> I think 8019902 tries to rectify this to some degree.
>
> Maybe part of the problem described in that CR is that G1 (and other
> collectors) do not update the various sizes (OldSize, NewSize,
> MaxNewSize and maybe others) after calculating them, but then use these
> variables for that anyway.
>
> Thomas
>
>