• Type: Enhancement
• Component: hotspot
• Sub-Component: runtime
• Affected Version: 9

• Priority: P1
• Status: Closed
• Resolution: Fixed

• Submitted: 2015-06-16
• Updated: 2017-07-26
• Resolved: 2015-08-14

JDK 9
9 b81Fixed

URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/927043f643bc User: lana Date: 2015-09-09 21:33:20 +0000 09-09-2015 URL: http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/927043f643bc User: coleenp Date: 2015-08-14 02:12:11 +0000 14-08-2015 2 more items from Kim Barrett: ------------------------------------------------------------------------------ src/share/vm/runtime/globals.cpp 1248 CommandLineFlagRange* range = CommandLineFlagRangeList::at(i); 1249 const char* name = range->name(); 1250 Flag* flag = Flag::find_flag(name, strlen(name), true, true); 1251 if (flag != NULL) { [Pre-existing code, though moved as part of this change.] I don't understand the need for the "if (flat != NULL) {" wrapper. Range objects are only created and registered with flags, so it shouldn't be possible for that test to ever fail. Note that the constraint processing doesn't have such a test. [Doing anything about this should be a separate RFE, and this question is really directed more toward Gerard rather than Sangheon.] ------------------------------------------------------------------------------ src/share/vm/runtime/globals.cpp 1213 bool CommandLineFlags::check_ranges() { 1214 //#define PRINT_RANGES_SIZES 1215 #ifdef PRINT_RANGES_SIZES ... 1274 static bool check_constraints(CommandLineFlagConstraint::ConstraintType constraint_type) { 1275 //#define PRINT_CONSTRAINTS_SIZES 1276 #ifdef PRINT_CONSTRAINTS_SIZES [More or less pre-existing code, though moved and updated as part of this change.] Do we really need this development cruft cluttering the code? [Doing anything about this should be a separate RFE, and this question is really directed more toward Gerard rather than Sangheon.] ------------------------------------------------------------------------------ 23-07-2015 Done - I will do RBT to make sure all is good an rev up a new webrev. 26-06-2015 Yes, please! It seems unsafe that the constraint checking functions have the ability to modify the parameter. 26-06-2015 The answer was that I originally thought that we could take the pointer to the value, so that in the event that it was outside the range, we could automatically adjust it, but we discussed it and decided that it was outside the JEP, however, I forgot to make this change. Should I change the functions to take the argument as a value as part of this webrev? 26-06-2015 What was the answer to my issue #2 where you pass a pointer to the value* in the constraint functions? ie: 31 Flag::Error AliasLevelConstraintFunc(bool verbose, intx* value) { You don't modify the value in the function. Was it necessary for the macro expansion to be a pointer? 25-06-2015 Issues raised by Kim have been addressed. 18-06-2015 Issues raised by Dmitry have been addressed. 18-06-2015 Issues raised by Coleen have been addressed. 17-06-2015 Issues found by Kim Barrett: >> src/share/vm/gc/g1/g1_globals.hpp >> 71 product(double, G1ConcMarkStepDurationMillis, 10.0, \ >> 72 "Target duration of individual concurrent marking steps " \ >> 73 "in milliseconds.") \ >> 74 range(1.0, (double)max_uintx) \ >> >> The new upper bound here seems like a completely random number to me. >> >> > The existing ad-hoc code just checked the min, and left the upper bound unchecked. Since we need an upper bound for range check, I had to provide some reasonable value and (double)max_uintx seemed a better choice than the mathematically correct DBL_MAX, which would require me to add #import <float.h> My opinion: <float.h> is just not big enough to be seriously concerned about. Using something that has some meaning and is not a very nearly randomly chosen value is more important. If we really want to avoid <float.h>, pick a stupidly large but “meaningful” upper bound like an hour (or 24 hours, or… ) or 10000 seconds or some such. Note that any change from DBL_MAX is at least theoretically a user-impacting change, so ought to go through the process for that. (Maybe that will push you to <float.h> :-) >> src/share/vm/runtime/globals.hpp >> 375 static const char* flag_error_str(Flag::Error error) { >> >> All other functions for the Flag are declared in the .hpp and defined >> in the .cpp file. I don't see any reason for this function to be >> different. > “flag_error_str” is a static method and I simply followed the pattern set by the other static methods (ie. “find_flag”, “fuzzy_match”). find_flag and fuzzy_match are declared in the .hpp and defined in the .cpp. >> src/share/vm/runtime/globals.hpp >> 1563 product(size_t, HeapSizePerGCThread, ScaleForWordSize(64*M), \ >> 1564 "Size of heap (bytes) per GC thread used in calculating the " \ >> 1565 "number of GC threads") \ >> 1566 range((uintx)os::vm_page_size(), max_uintx) \ >> >> Type is size_t, but range values are using uintx. >> >> > The original ad-hoc code used "(uintx)os::vm_page_size()” for the min, but did not check the upper bound, so I provided max_uintx for the max. > > Also, HeapSizePerGCThread is assigned to a local variable of type “uintx” and there is no such thing as max_size_t (?) The code I’m looking at (hs-rt tip) uses (size_t)os::vm_page_size() for the min. That change was relatively recent: 8074459: Flags handling memory sizes should be of type size_t Maybe an incorrect “merge” with that change set in your changes? >> src/share/vm/runtime/globals.hpp >> 1846 product(size_t, MarkStackSizeMax, NOT_LP64(4*M) LP64_ONLY(512*M), \ >> 1847 "Maximum size of marking stack") \ >> 1848 range(1, (max_jint - 1)) \ >> >> That's a very odd looking maximum. >> >> >> > It comes from the original ad-hoc code. That doesn’t make it any less odd looking. I think it is worth an RFE to understand that value and add a comment explaining it (or changing it to something less surprising, if that’s warranted). Or at least a comment questioning the value. Not that I think anyone’s going to look at it anytime soon. >> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp >> 100 #define UseConcMarkSweepGCWorkaroundIfNeeded(initial, max) { \ >> 101 if ((initial == 7) && (max == 6)) { \ >> 102 return Flag::SUCCESS; \ >> 103 } \ >> 104 } >> >> The do/while(0) macro idiom is the usual way to encapsulate >> statements. It took me a couple of tries to parse this, where it >> would have been immediately obvious with the do/while(0) idiom. >> >> >> > Can you please provide a concrete example of how I should use "do/while(0)” here? I don't understand how that would help with the readability - we’re talking here about a simple macro that short-circuits a method if the “if” statement passes. The expansion presently wraps the “if” statement in an extra level of braces, to avoid leaving a dangling if. That’s good, but I found it non-obvious, perhaps in part due to the formatting. But the “usual idiom” for that sort of thing is the do/while(0) idiom, e.g #define UseConcMarkSweepGCWorkaroundIfNeeded(initial, max) \ do { \ if (initial == 7) && (max == 6)) { \ return Flag::SUCCESS; \ } \ } while (0) If written that way, it would have been immediately obvious (to me) what was being done, since do/while(0) is a "well-known” and easily recognized pattern chunk. >> src/share/vm/runtime/globals.cpp >> >> In the various apply_constraint_and_check_range_TYPE functions, the >> present pattern seems to be to find and check the range (if it >> exists), then find and apply the constraint (if it exists), and then >> combine the results. >> >> I think it might be better to not apply the constraint if there is a >> range check failure. That way, constraint functions can be written to >> assume that the values have been range checked, and only need to worry >> about whatever additional constraints might exist, without the need to >> potentially deal with out of range values. >> >> Whichever behavior is used should be documented as part of the >> interface to this facility, since it affects how one writes constraint >> functions. It might also, in some cases, affect whether it is useful >> to provide a range spec in conjunction with a constraint function. >> >> Such a change might also eliminate the need for the get_status_error >> helper. >> >> [Obviously this is pretty high priority if handled as a followup >> change, since it has a somewhat significant impact on how one writes >> constraint functions.] >> >> > I added this behavior to address David’s concern from webrev 2, but to be honest I had doubts about it. > > Originally, I had the code checking the ranges first, and only checking the constraints if the range check passed. However, that required me to change "test/runtime/CompressedOops/ObjectAlignment.java” and "/test/runtime/contended/Options.java”, by removing the cases that checked both ranges and constraints, which David pointed out as loosing functionality. The previous ad-hoc code was free to do whatever it wanted and in those 2 cases (ObjectAlignmentInBytes and ContendedPaddingWidth) the test was free to check both the range and constraint at the same time. Sorry, I overlooked or forgot the earlier discussion in this area. > So I tried to do the same in my framework, but that made the code a bit more complex (ie. need for “get_status_error” helper). > > Most importantly, however, I much prefer the design where the constraint check is only attempted if the range passes (ie. constraint is guaranteed that the value is within the range) > > I will revert the code back to what it was like in webrev 2, and these 2 tests will technically loose some functionality, however, there are specific test cases in each of the tests that do exercise both the constraint and the range, though one at the time. 16-06-2015 Issues found by Dmitry Dmitriev: I noticed 2 typos for new int and uint types in src/share/vm/runtime/commandLineFlagRangeList.cpp: 45 Flag::Error check_int(int value, bool verbose = true) { 46 if ((value < _min) || (value > _max)) { 47 if (verbose == true) { 48 jio_fprintf(defaultStream::error_stream(), 49 "int %s=%d is outside the allowed range [ %d ... D ]\n", 50 name(), value, _min, _max); 51 } On line 49 at the end - "%d" must be instead of "D". 101 Flag::Error check_uint(uint value, bool verbose = true) { 102 if ((value < _min) || (value > _max)) { 103 if (verbose == true) { 104 jio_fprintf(defaultStream::error_stream(), 105 "uintx %s=%u is outside the allowed range [ %u ... %u ]\n", 106 name(), value, _min, _max); 107 } On line 105 - "uint" should be instead of "uintx" before "%s=%u". 16-06-2015 Issues found by Coleen Phillimore: http://cr.openjdk.java.net/~gziemski/8059557_rev3/src/share/vm/runtime/commandLineFlagRangeList.cpp.html 87 st->print("[ "INTX_FORMAT_W(-25)" ... "INTX_FORMAT_W(25)" ]", _min, _max); In some C++15 code the lack of space between INTX_FORMAT and the string is an error, can you fix these? I thought you had this comment already but either I have an old webrev or you missed this one. http://cr.openjdk.java.net/~gziemski/8059557_rev3/src/share/vm/runtime/commandLineFlagConstraintsCompiler.cpp.html 31 Flag::Error AliasLevelConstraintFunc(bool verbose, intx* value) { In the constraint functions, the value parameter isn't set by the function. Why is it a pointer rather than a value? Is this required for instantiating the macro for the constraint functions? If so, could you make the pointers const or would that also not instantiate correctly with the macro? http://cr.openjdk.java.net/~gziemski/8059557_rev3/src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp.html 33 if (verbose == true) { 34 jio_fprintf(defaultStream::error_stream(), 35 "ObjectAlignmentInBytes=" INTX_FORMAT " must be power of 2\n", 36 *value); 37 } It seems like a printing function like print_command_line_error(verbose, const char* fmt, ...) would be nice here since this code pattern exists in a lot of places and it'll catch someone from forgetting the if (verbose) check. For booleans, the style is if (verbose) rather than if (verbose == true), which I think people have pointed out. If the thing in the conditional is a pointer, it should be explicitly checked for NULL: if (ptr == NULL) or ideally if (NULL == ptr). 16-06-2015