Request for review: 6521376: MaxTenuringThreshold and AlwayTenure/NeverTenure consistency

[Tao Mao]

Hi all,

The webrev is updated.
http://cr.openjdk.java.net/~tamao/6521376/webrev.05/

The suggestions for print formatting and assertions are taken. Good 
points! Thanks, Jon.:-)

Note the behaviors of -XX:+PrintTenuringDistribution

(1) Parallel GC

Before:
java -XX:+PrintTenuringDistribution TestFullGC

Desired survivor size 11010048 bytes, new threshold 7 (max 15)

After:
java -XX:+PrintTenuringDistribution TestFullGC

Desired survivor size 524288 bytes, new threshold 7 (max threshold 15)

(2) Other collectors

Before:
java -XX:+UseSerialGC -XX:+PrintTenuringDistribution TestFullGC

Desired survivor size 65536 bytes, new threshold 1 (max 15)
- age   1:     131064 bytes,     131064 total

After:
java -XX:+UseSerialGC -XX:+PrintTenuringDistribution TestFullGC

Desired survivor size 65536 bytes, new threshold 1 (max threshold 15)
- age   1:     131072 bytes,     131072 total



On 11/14/13 8:32 AM, Jon Masamitsu wrote:
> Tao,
>
> http://cr.openjdk.java.net/~tamao/6521376/webrev.04/src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp.udiff.html
>
> +         gclog_or_tty->print_cr("Desired survivor size " SIZE_FORMAT " bytes, new threshold %u (max threshold %u)",
>
> What do you think of using UINTX_FORMAT instead of %u?  Just a little 
> bit future proof?
>
> http://cr.openjdk.java.net/~tamao/6521376/webrev.04/src/share/vm/gc_implementation/shared/ageTable.cpp.udiff.html
>
> Consider an assertion to check that MaxTenuringThreshold has one of 
> the two expected values.
> +  uint result;
> +
> +  if (AlwaysTenure || NeverTenure) {
> +    result = MaxTenuringThreshold;
>
> assert(MaxTenuringThreshold == 0 || MaxTenuringThreshold ==markOopDesc::max_age + 1,
>    errmsg("MaxTenuringThreshold should be either 0 or " UINTX_FORMAT " and is " UINTX_FORMAT,
>           MaxTenuringThreshold);
>
> +  } else {
>
> +      gclog_or_tty->print_cr("Desired survivor size " SIZE_FORMAT " bytes, new threshold %u (max threshold %u)",
>
> Use UINTX_FORMAT?
>
> That's all.  Rest looks good.
>
> Jon
>
> On 11/13/13 4:36 PM, Tao Mao wrote:
>> Hi all,
>>
>> New webrev uploaded.
>> http://cr.openjdk.java.net/~tamao/6521376/webrev.04/
>>
>> Please see inline, Thomas.
>>
>> Thanks.
>> Tao
>>
>> On 11/12/13 2:42 AM, Thomas Schatzl wrote:
>>> Hi,
>>>
>>>    sorry for the late response...
>>>
>>> On Wed, 2013-11-06 at 16:43 -0800, Tao Mao wrote:
>>>> Hi all,
>>>>
>>>> A new webrev is updated
>>>> http://cr.openjdk.java.net/~tamao/6521376/webrev.03/
>>>>
>>>> It includes a thorough test, which is one good way to examine the
>>>> changeset.
>>> Agree :)
>>>> Changeset:
>>>>
>>>> One point I want to discuss here is the validity of setting
>>>> -XX:MaxTenuringThreshold=16. The reason why I'd consider it valid is:
>>>> when NeverTenure is set, MaxTenuringThreshold is set to 16 while
>>>> setting MaxTenuringThreshold=15 doesn't make sense here. Thus,
>>>> MaxTenuringThreshold=16 needs to be considered a valid setting.
>>> I think I am fine with this.
>>>
>>> As for the test in TestObjectTenuringFlags: I think it would be more
>>> clear if every test were a single method call instead of the
>>>
>>> vmOpts.clear();
>>> collections.addAll( ...test settings... );
>>> shouldFail = ...
>>> expectedFlags = ...
>>> runTest(...)
>>>
>>> e.g. as
>>>
>>> runTest(new String[] { test settings }, true /* should_fail */, new
>>> ExpectedTenuringFlags( ... ));
>> Done.
>>>
>>> I do not really like the use of the "vmOpts" global both in the main()
>>> method and the ""runTenurinigFlagsConsistencyTest()". I do not think
>>> there is need to save memory here, and that would save the test to 
>>> reset
>>> the "vmOpts" variable all the time.
>> Done.
>>> I do not think the test needs to basically rethrow the RuntimeException
>>> from checkTenurinigFlagsConsistency in checkTenurinigFlagsConsistency.
>>> The test could just let it fall through (without try-catch).
>> Done.
>>>
>>> Also checkTenurinigFlagsConsistency could fail fast on any incorrect
>>> consistency check, with an explicit message for every type of failure
>>> (e.g. "expected AlwaysTenure to be<alwaysTenure value>, is<actual
>>> value>  instead"), instead of accumulating the error using the&&
>>> operator.
>> Done.
>>>
>>> The stack trace in the error will give you the failing test quickly, 
>>> but
>>> it's not bad to show the VM parameters that lead to the error.
>> Chose not to do so. The stack trace should be enough to track down.
>>>
>>> There is also a typo in the "runTenurinigFlagsConsistencyTest" test
>>> name, an additional "i". The same typo in
>>> "checkTenurinigFlagsConsistency".
>> Done.
>>>
>>> Otherwise looks good.
>>>
>>> Thanks,
>>> Thomas
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131114/cb1f745f/attachment.htm>