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

Jon Masamitsu jon.masamitsu at oracle.com
Fri Nov 15 17:20:21 UTC 2013


Tao,

Looks good.  Thanks for the changes.

Jon

On 11/14/2013 4:04 PM, Tao Mao wrote:
> 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.java.net/pipermail/hotspot-gc-dev/attachments/20131115/d18eca17/attachment.htm>


More information about the hotspot-gc-dev mailing list