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

Thomas Schatzl thomas.schatzl at
Tue Nov 12 10:42:00 UTC 2013


  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

collections.addAll( ...test settings... );
shouldFail = ...
expectedFlags = ...

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

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

Otherwise looks good.


More information about the hotspot-gc-dev mailing list