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

[Jon Masamitsu]

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/bfb7f3f3/attachment.htm>