RFR 8078555(M): GC: implement ranges (optionally constraints) for those flags that have them missing
sangheon.kim
sangheon.kim at oracle.com
Fri Aug 21 06:32:12 UTC 2015
Hi Gerard,
Thank you for reviewing this.
On 08/20/2015 11:57 AM, gerard ziemski wrote:
> 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)) {
Okay, I fixed as you suggested.
>
>
> #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
Done.
>
>
> #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.
Yes, this makes better look.
Done.
>
>
> #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));
Done.
>
>
> #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 ;-)
Hmm.
Okay, let's talk more from your review. :)
http://cr.openjdk.java.net/~sangheki/8078555/webrev.02/
http://cr.openjdk.java.net/~sangheki/8078555/webrev.02_to_01
Thanks,
Sangheon
>
>
> 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