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

[Tao Mao]

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
>
>