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

[sangheon.kim]

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.
I can revert to webrev.02 and change groups to AfterMemoryInit.

>
> 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.

Will you review them again or okay to go (and handle from separate RFE)?

Thanks,
Sangheon


>