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

[Thomas Schatzl]

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( ... ));

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.

There is also a typo in the "runTenurinigFlagsConsistencyTest" test
name, an additional "i". The same typo in
"checkTenurinigFlagsConsistency".

Otherwise looks good.

Thanks,
Thomas