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

[Kim Barrett]

On Oct 1, 2015, at 6:27 PM, sangheon.kim <sangheon.kim at oracle.com> wrote:
> On 10/01/2015 02:16 PM, Kim Barrett wrote:
>> 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.
> Right, that is also possible solution.
> And I already tried this approach of changing group to AfterMemoryInit and it also works fine.
> (JPRT and running TestOptionsWithRanges.java for all platforms including embedded)
> But the reason of stopping this approach was current webrev.03 also can fix the problem.
> In addition, personally adding more options to AfterMemoryInit group seems weakening validation framework as sooner validation check seems better for me. For this case, I don't think they are weakening though.
> But I don't have strong opinion on this.

My understanding was that the rationale for the different constraint
groups was to allow for options that are "finalized" at different
stages of the initialization process, so that the constraints would be
applied to the "final" values (subject to later management-API-based
modifications).

I think that the webrev.03 approach (using FLAG_IS_CMDLINE in the
constraint function) tightly couples the constraint function with the
(textually distant) ergonomics implementation in a way that I'm not
very comfortable with.  I also think that in that form it could be
moved to the AtParse category.  In fact, it seems likely that we could
rewrite all of the constraint functions in this style, in which case
why do we have the different constraint category stages?

> I can revert to webrev.02 and change groups to AfterMemoryInit.

I would prefer that.

>> But this discussion is making me question all of the FLAG_IS_xxx uses
>> in commandLineFlagConstraintsGC.cpp.
> For some options, they are automatically added to validate.
> (e.g. SurvivorRatio for CMS GC )
> So FLAG_IS_xxx is needed for some cases.

The (pre-existing) ergonomics around SurvivorRatio seem quite strange
to me.  Why is the behavior be different when the value is defaulted
vs when it is explicitly specified with the same value as the default?

I don't expect anything to be done about this right now, but I think
this does help make the case for using FLAG_IS_CMDLINE in the pause
time options under discussion.