RFR 8078555(M): GC: implement ranges (optionally constraints) for those flags that have them missing
gerard ziemski
gerard.ziemski at oracle.com
Thu Aug 20 18:57:23 UTC 2015
hi Sangheon,
I won't pretend to understand all the GC flags and their meanings, but I
tried my best and it looks good, but I have a few small nitpicks:
#1
----------------------------------
src/share/vm/runtime/arguments.cpp
587 if (errno != 0 || *end != 0) {
Would be nice to have extra brackets?
587 if ((errno != 0) || (*end != 0)) {
#2
-----------------------------------------------------
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
77 if (UseConcMarkSweepGC && (value > max_jint / 10)) {
Can we perhaps add some extra brackets around math operations, like?
77 if (UseConcMarkSweepGC && (value > (max_jint / 10))) {
similarly lines 81, 191, 363, 367, 533, 567, 571, 580
#3
-----------------------------------------------------
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
442 "MaxGCPauseMillis (" UINTX_FORMAT ")
must be less than “
443 "GCPauseIntervalMillis (" UINTX_FORMAT
")\n”,
Technically it should be:
442 "MaxGCPauseMillis (" UINTX_FORMAT ")
must be “
443 "less than GCPauseIntervalMillis ("
UINTX_FORMAT ")\n",
similarly other messages where "must be” should end the first’s message
text line for consistency.
#4
-----------------------------------------------------
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
363 if (UseConcMarkSweepGC && (value > max_jint / ParallelGCThreads)) {
Should we perhaps cast up?
363 if (UseConcMarkSweepGC && (value > ((uintx)max_jint /
(uintx)ParallelGCThreads))) {
And also change:
366 "less than or equal to ergonomic maximum
(" INT32_FORMAT ")\n",
367 value, max_jint / ParallelGCThreads);
to:
366 "less than or equal to ergonomic maximum
(" UINTX_FORMAT ")\n",
367 value, ((uintx)max_jint /
(uintx)ParallelGCThreads));
#5
I have noticed that none of the flags used "AtParse" in their
constraints and I am again starting to think that "AtParse" is not quite
what we want, but I will get into that later with my own check-in ;-)
cheers
On 08/20/2015 01:31 AM, sangheon.kim wrote:
> Hi Kim and Dmitry,
>
> Here's new webrev.
>
> http://cr.openjdk.java.net/~sangheki/8078555/webrev.01/
> http://cr.openjdk.java.net/~sangheki/8078555/webrev.01_to_00/
>
> And let me correct one of my answers to Kim.
>>> I see that the Check helper is being used both here and for
>>> MaxHeapSize constraint function. I think the helper needs to be
>>> called with **both** values, and not refer to the current values at
>>> all. The SoftRefXXX checker would call Check with the argument value
>>> and the current value of MaxHeapSize. The MaxHeapSize checker would
>>> call Check with the current value of SoftRefXXX and the argument
>>> value.
>>> Oh, right.
>>> I will fix this.
>> Oh, right.
>> I will fix this.
> As you already mentioned, validation check can be done for only one flag
> at a time.
> So helper function can't called with BOTH new values.
> But I think there's no problem with this as eventually these new values
> will be compared.
> e.g. flagA = 10(default 5), flagB = 20(default 5) and constraint: flagA
> * flagB <= 150
> constraint functions will be called for their declared order.
> when constraint function for flagA is called: 10(new flagA) * 5(default
> flagB) -> PASS
> when constraint function for flagB is called: 10(new flagA) * 20(new
> flagB) -> FAIL.
>
> Thanks,
> Sangheon
>
>
> On 08/19/2015 03:58 PM, sangheon.kim wrote:
>> Hi Kim,
>>
>> Thank you for reviewing this.
>>
>> On 08/19/2015 02:59 PM, Kim Barrett wrote:
>>> On Aug 19, 2015, at 12:25 AM, sangheon.kim <sangheon.kim at oracle.com>
>>> wrote:
>>>> Hi all,
>>>>
>>>> Please review this patch for command-line validation for GC flags.
>>>> This includes adding ranges and implementing constraint functions
>>>> for GC flags.
>>>>
>>>> […]
>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8078555
>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8078555/webrev.00/
>>>> Testing: JPRT, UTE(vm.quick-pcl), tests at test/runtime/CommandLine
>>>> and local tests[1].
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/gc/g1/g1_globals.hpp
>>> 87 product(size_t, G1SATBBufferSize,
>>> 1*K, \
>>> 88 "Number of entries in an SATB log
>>> buffer.") \
>>> 89 range(1,
>>> max_uintx) \
>>>
>>> Shouldn't that be max_size_t? Oh, wait; we don't have that. Maybe we
>>> should.
>> It would be nice to have.
>> Let me handle from follow up CR.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/runtime/arguments.cpp
>>> 583 static bool set_fp_numeric_flag(char* name, char* value,
>>> Flag::Flags origin) {
>>> 584 errno = 0;
>>> 585 double v = strtod(value, NULL);
>>> 586 if (errno == ERANGE) {
>>> 587 return false;
>>> 588 }
>>>
>>> You and Dmitry already discussed checking the endptr. Good catch,
>>> Dmitry.
>>>
>>> I suggest returning false for any non-zero errno value.
>> Okay, I will change to (errno != 0).
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/runtime/arguments.cpp
>>> 722 #define NUMBER_RANGE "[0123456789eE+]"
>>>
>>> Missing "-" for negative exponents.
>> This definition is used for below decimal point. So we don't need '-'
>> here.
>> Currently we can reach strtod function above with '-1.1'.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>> 48 static Flag::Error
>>> ParallelGCThreadsAndCMSWorkQueueDrainThreshold(bool verbose) {
>>> 61 Flag::Error ParallelGCThreadsConstraintFunc(uint value, bool
>>> verbose) {
>>> 90 Flag::Error ConcGCThreadsConstraintFunc(uint value, bool
>>> verbose) {
>>>
>>> These are associated with collectors that are excluded when
>>> INCLUDE_ALL_GCS is false. The bodies of these should be conditionally
>>> included based on that macro, and they should just return
>>> Flag::SUCCESS when the macro is false.
>> Okay.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>> 142 Flag::Error OldPLABSizeConstraintFunc(size_t value, bool
>>> verbose) {
>>> 143 if (UseConcMarkSweepGC) {
>>>
>>> The UseConcMarkSweepGC clause should be conditionalized out if
>>> INCLUDE_ALL_GCS is false.
>>>
>>> Peeking ahead, it looks like there are quite a few constraint
>>> functions that should be similarly conditionalized.
>> Okay.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>> 183 static Flag::Error
>>> CheckMaxHeapSizeAndSoftRefLRUPolicyMSPerMB(bool verbose) {
>>> ...
>>> 196 Flag::Error SoftRefLRUPolicyMSPerMBConstraintFunc(intx value,
>>> bool verbose) {
>>> 197 return CheckMaxHeapSizeAndSoftRefLRUPolicyMSPerMB(verbose);
>>> 198 }
>>>
>>> This doesn't look right. The Check helper function is using the
>>> current value of SoftRefLRUPolicyMSPerMB, and doesn't have access to
>>> the prospective new value, since the constraint function isn't passing
>>> it.
>>>
>>> I see that the Check helper is being used both here and for
>>> MaxHeapSize constraint function. I think the helper needs to be
>>> called with *both* values, and not refer to the current values at
>>> all. The SoftRefXXX checker would call Check with the argument value
>>> and the current value of MaxHeapSize. The MaxHeapSize checker would
>>> call Check with the current value of SoftRefXXX and the argument
>>> value.
>> Oh, right.
>> I will fix this.
>>
>>>
>>> Otherwise, updates that validate the value before performing the
>>> assignment will be using the old value for validation, rather than
>>> checking the new value as intended.
>>>
>>> This also brings up an interesting limitation of the present
>>> mechanism. If two options are related in some way, both need to have
>>> constraint checking functions to ensure the relationship remains valid
>>> after initial option checking. But this means that the initial option
>>> checking will check the relationship twice, and if there is a problem
>>> it will be reported twice. I don't have any easy suggestions for how
>>> to improve on this situation though.
>> Right.
>> This is current limitation.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/runtime/globals.hpp
>>> 1854 range(0,
>>> MarkStackSizeMax) \
>>>
>>> Does this actually work? I'm not sure when the range values are
>>> evaluated. I suspect this wouldn't work properly in the face of later
>>> updates to MarkStackSizeMax.
>> Correct.
>> I will change their order to be 'MarkStackSizeMax' first.
>>
>> I will send next version of webrev soon.
>>
>> Thanks,
>> Sangheon
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>>
>
>
>
More information about the hotspot-dev
mailing list