RFR 8134995(M): [REDO] GC: implement ranges (optionally constraints) for those flags that have them missing

[Kim Barrett]

On Oct 1, 2015, at 2:20 PM, sangheon.kim <sangheon.kim at oracle.com> wrote:
> 
> On 10/01/2015 10:52 AM, Kim Barrett wrote:
>> On Oct 1, 2015, at 12:24 PM, sangheon.kim <sangheon.kim at oracle.com> wrote:
>>> Hi all,
>>> 
>>> During this code review, newly added gc test found a problem of 1 gc constraint function(MaxGCPauseMillisConstraintFunc).
>>> 
>>> So I updated it and related function (GCPauseIntervalMillisConstraintFunc).
>>> - Added checking for MaxGCPauseMillis and GCPauseIntervalMillis whether these are set via commandline or not to proceed the constraint function logic.
>>> 
>>> http://cr.openjdk.java.net/~sangheki/8134995/webrev.03
>>> http://cr.openjdk.java.net/~sangheki/8134995/webrev.03_to_02/
>>> 
>>> * Testing: JPRT and RBT(to manually test runtime/CommandLine for embedded)
>> This change doesn't seem right.  There is a relationship between these
>> two values that should hold after all the argument processing is done,
>> and it doesn't matter where the values came from.  All of the
>> FLAG_IS_CMDLINE checks (including those from earlier versions) in
>> these constraint functions now seem questionable to me.
>> 
>> It seems to me that if there is an unexpected test failure here, then
>> the problem is that some "ergonomic" setting of these values is
>> occurring after the constraint function is being called.  Or
>> alternatively, the ergnonomic settings are not right in some cases.
> The reason of test failure is simply can't pass the constraint function.
> The test is setting only 'MaxGCPauseMillis=30000' which means GCPauseIntervalMillis will have default value of 0(zero means set ergonomically).
> Without constraint function, G1CollectorPolicy will set GCPauseIntervalMillis=30001.
> No problem with the test.
> 
> For G1, 'MaxGCPauseMillis >= GCPauseIntervalMillis' is one of the condition should be kept.
> But the constraint function is trying to compare 30000(set via cmd line) vs. 0(default).
> The intend of using FLAG_IS_CMDLINE() is to avoid comparison with default value which means set ergonomically.
> 
> All other cases that using FLAG_IS_CMDLINE() is for same purpose, to avoid comparison with default value.

The constructor for G1CollectorPolicy is responsible for some of the
ergonomic setup, most particularly the two options in question
(MaxGCPauseMillis and GCPauseIntervalMillis).

I think that constraint functions are not supposed to be run until
after the associated ergonomics have been applied. But the constraint
functions for these options are being assigned to the AfterErgo group,
which is run before the heap is created, which includes creating the
collector policy.

One possible fix might be to assign these constraint functions to the
AfterMemoryInit group.  That certainly seems appropriate for
GCPauseIntervalMillis, which appears to be G1-specific.  And since
MaxGCPauseMillis has no additional constraints when not using G1, such
a move would also work for it.

But this discussion is making me question all of the FLAG_IS_xxx uses
in commandLineFlagConstraintsGC.cpp.