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

[Tao Mao]

new webrev:
http://cr.openjdk.java.net/~tamao/6521376/webrev.02/

Please see inline.

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 most recent gc repo behavior:
$java -XX:InitialTenuringThreshold=5 -XX:+AlwaysTenure -version

runs without abort. It seems to "silently" accept the inconsistency of 
ITT and MTT because the code doesn't sync MTT with AlwaysTenure yet. 
Thus, it does

In my changeset, with the sync of ITT and MTT, the behavior goes:

$java -XX:+AlwaysTenure -XX:InitialTenuringThreshold=5 -version
(or, $java -XX:InitialTenuringThreshold=5 -XX:+AlwaysTenure -version)

InitialTenuringThreshold of 5 is invalid; must be between 0 and 0
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

This is the expected behavior so that we would warn the user change ITT 
(or, simply remove it if s/he wishes to) to cope with 
"-XX:+AlwaysTenure" (i.e. MTT=0).

I don't see any need to sync ITT and MTT in other silent way.
>
> - 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.
Fixed. Thank you for pointing out.
> - 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.
I'll write jtreg later once I consolidate the behaviors here.
>
> 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.
I provide the expected behavior table below (mainly copied and modified 
from the first email I sent)

(1) Setting -XX:+NeverTenure/-XX:+AlwaysTenure

(1-a) -XX:+NeverTenure
->
NeverTenure=true; AlwaysTenure=false; 
MaxTenuringThreshold=markOopDesc::max_age+1 (i.e. 16, use the value 
instead below);

(1-b) -XX:+AlwaysTenure
->
NeverTenure=false; AlwaysTenure=true; MaxTenuringThreshold=0;

(2) Setting MaxTenuringThreshold

(2-a) MaxTenuringThreshold == 0
->
NeverTenure=false; AlwaysTenure=true;

(2-b) 0 < MaxTenuringThreshold < 16
->
NeverTenure=false; AlwaysTenure=false;

(2-c) MaxTenuringThreshold = 16
->
NeverTenure=true; AlwaysTenure=false;

(2-d) MaxTenuringThreshold > 16
->
VM should abort due to the check

2164   status = status&&  verify_percentage(YoungGenerationSizeSupplement, "YoungGenerationSizeSupplement");
2165   status = status&&  verify_percentage(TenuredGenerationSizeSupplement, "TenuredGenerationSizeSupplement");
2166
*2167   status = status&&  verify_interval(MaxTenuringThreshold, 0, markOopDesc::max_age + 1, "MaxTenuringThreshold");*
2168   status = status&&  verify_interval(InitialTenuringThreshold, 0, MaxTenuringThreshold, "InitialTenuringThreshold");
2169   status = status&&  verify_percentage(TargetSurvivorRatio, "TargetSurvivorRatio");


*(3) -XX:-NeverTenure/-XX:-AlwaysTenure only need to take care of 
themselves and don't need the flag consistency maintenance here.
>
>> 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.
See the behavior summary above.
>
> 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...
I agree.
> Thanks,
> Thomas
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130702/e33c1bc7/attachment.htm>