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

[Tao Mao]

Sorry for the inconvenience. Not sure what happened to generating the 
diff files. I've uploaded the CR in correct format to
http://cr.openjdk.java.net/~tamao/6521376/webrev.01/

Thomas: I'm still walking through your comments and will get back to you 
later.

Thanks.
Tao

On 6/24/13 6: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
>
>
>