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

[Charlie Hunt]

Hi Thomas,

A comment on your question:
> I am not really convinced that setting a tenuring threshold >= 16 makes
> a lot of sense, but I cannot find a reason why 16 is better than any
> other higher value, i.e. the reason for silently setting everything above
> that value to 16.
> 
> Maybe others can comment on the expected behavior better than me.

Prior to biased locking stealing one of the age bits, max tenuring threshold could be set (legitimately) as high as 31.  Surprisingly there are some apps running with those older JVMs that pre-date biased locking.

This being said .... fwiw, I would find value in having the JVM report that a value is in excess of what can be supported by the JVM, or that a command line option is no longer supported, or available like what we see reported if you specify  -XX:+UseCompressedStrings in Java 7, (this case is a bit different than what's under consideration though).

hths,

charlie ...
 
On Jun 24, 2013, at 8:50 AM, Thomas Schatzl wrote:

> 
> Hi,
> 
> On Sat, 2013-06-22 at 20:03 -0700, Tao Mao wrote:
>> 6521376: MaxTenuringThreshold and AlwayTenure/NeverTenure consistency
>> 
>> webrev:
>> http://cr.openjdk.java.net/~tamao/6521376/webrev.00/
> 
> The webrev includes a complete psScavenge.cpp. To get to the actual
> changes, you have to manually diff it. Could you please minimize the
> change for that file and re-upload the webrev?
> 
> I manually diffed that files with the current version to find the
> actual changes...
> 
> Some notes:
> 
> - the change does not maintain consistency of MaxTenuringThreshold with
> the InitialTenuringThreshold variable.
> Running the VM with -XX:InitialTenuringThreshold=5 -XX:+AlwaysTenure
> will give an error (note that InitialTT is set before AlwaysTenure); the
> other way the VM should give an error.
> 
> - the change in AgeTable::compute_tenuring_threshold() effectively also
> disables the output of PrintTenuringDistribution and updates related to
> UsePerfData. Particularly the latter seems to be a bug.
> 
> - please provide test cases that cover all possible classes of
> combinations for these variables (Initial/MaxTenuringThreshold,
> Always/NeverTenure). You could use the test case for 8014765 as basis.
> 
> There are already some helper classes in the test/gc/arguments directory
> that e.g. allow you to parse the output of -XX:+PrintFlagsFinal to verify
> them.
> 
> It may help (also us) to also provide one or more small tables with expected behavior given input values, and use that to catch all possible situations.
> 
>> changeset: For all collectors,
>> 
>> (1) Setting -XX:+NeverTenure/-XX:+AlwaysTenure
>> "-XX:+NeverTenure"
>> ->
>> NeverTenure=true; AlwaysTenure=false;  
>> MaxTenuringThreshold=markOopDesc::max_age+1 (i.e. 16, use the value 
>> instead below);
>> 
>> "-XX:+AlwaysTenure"
>> ->
>> NeverTenure=false; AlwaysTenure=true;  MaxTenuringThreshold=0;
>> 
>> (2) Setting MaxTenuringThreshold
>> 
>> (2-a) MaxTenuringThreshold == 0
>> if (MaxTenuringThreshold == 0):
>> "the cap equals 0" implies AlwaysTenure=true, so set flags accordingly 
>> (NeverTenure=false; AlwaysTenure=true).
>> 
>> (2-b) MaxTenuringThreshold > 0
>> But, considering the ergonomics for tenuring threshold (see 
>> PSAdaptiveSizePolicy::compute_survivor_space_size_and_threshold(), 
>> CMSAdaptiveSizePolicy::compute_survivor_space_size_and_threshold() and 
>> ageTable::compute_tenuring_threshold()), we will adjust tenure_threshold 
>> up and down but below the cap MaxTenuringThreshold during the 
>> application run.
>> 
>> Thus, setting MaxTenuringThreshold >= 16 would not necessarily imply 
>> that the user wants to apply NeverTenure to the VM for the real 
>> tenure_threshold may need to go below MaxTenuringThreshold as the 
>> process stats suggest.
>> 
>> With that said,
>> if (MaxTenuringThreshold > 0):
>> I simply set NeverTenure/AlwaysTenure to false. (neither NeverTenure nor 
>> NeverTenure)
>> and if (MaxTenuringThreshold > 16): set MaxTenuringThreshold to 16 as it 
>> is unnecessary to be larger.
> 
> Some notes:
> 
> 0 <= MaxTenuringThreshold < 16:
> Behavior as before, seems okay.
> 
> MaxTenuringThreshold >= 16:
> This is a change to previous behavior. Previously the VM would have
> simply exited, telling the user that he passed an illegal value, while
> the VM now silently sets the value to 16.
> 
> Looking at the CR which gives some example about the use, a value of 16
> may be useful, i.e. never tenure for a while.
> 
> Both compute_survivor_space_size_and_threshold() methods also change the
> tenuring thresholds incrementally: so a tenuring threshold > 16 may be
> useful (I do not know). E.g. to keep the tenuring threshold high during
> startup.
> 
> I am not really convinced that setting a tenuring threshold >= 16 makes
> a lot of sense, but I cannot find a reason why 16 is better than any
> other higher value, i.e. the reason for silently setting everything above
> that value to 16.
> 
> Maybe others can comment on the expected behavior better than me.
> 
>> *(3) -XX:-NeverTenure/-XX:-AlwaysTenure only need to take care of 
>> themselves and don't need the flag consistency maintenance here.
>> 
>> *******************************************************************************
>> Take a little convoluted case for example: suppose that we have the 
>> following gc options "-XX:+NeverTenure -XX:+MaxTenuringThreshold=18" in 
>> this particular order.
>> 
>> When we first parse "-XX:+NeverTenure", we would set: NeverTenure=true; 
>> AlwaysTenure=false;  MaxTenuringThreshold=16.
>> 
>> But when we encounter the second option "-XX:+MaxTenuringThreshold=18", 
>> we would think the user knows about gc ergonomics for tenuring threshold 
>> and wants MaxTenuringThreshold to just be a cap for the ergonomics 
>> rather than wants to (implicitly) use NeverTenure. So, we would set 
>> NeverTenure=false; AlwaysTenure=false; and finally 
>> MaxTenuringThreshold=16 to reflect the cap maximum.
>> *******************************************************************************
> 
> Instead of guessing the user's intent it may sometimes be better to just
> tell the user that the given options do not make sense imo...
> 
> Thanks,
> Thomas
> 
> 
>