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

Tao Mao tao.mao at
Thu Nov 14 00:36:50 UTC 2013

Hi all,

New webrev uploaded.

Please see inline, Thomas.


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
>> 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( ... ));
> 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.
> 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).
> 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.
> 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".
> Otherwise looks good.
> Thanks,
> Thomas

More information about the hotspot-gc-dev mailing list