RFR (M): 8112746: Followup to JDK-8059557 (JEP 245: Validate JVM Command-Line Flag Arguments)
gerard ziemski
gerard.ziemski at oracle.com
Tue Aug 4 17:25:05 UTC 2015
Thank you for the review.Comments in-line.
On 08/03/2015 03:26 PM, Kim Barrett wrote:
> On Aug 3, 2015, at 12:11 PM, gerard ziemski <gerard.ziemski at oracle.com> wrote:
>> On 07/31/2015 08:15 PM, Kim Barrett wrote:
>>> On Jul 31, 2015, at 2:04 PM, gerard ziemski <gerard.ziemski at oracle.com>
>>>> https://bugs.openjdk.java.net/browse/JDK-8122937
>>>>
>>>>
>>>> bug:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8112746
>>>>
>>>>
>>>> webrev:
>>>>
>>>> http://cr.openjdk.java.net/~gziemski/8112746_rev2/
>>> Not a full review, for reasons that will be discussed below. I
>>> decided to stop and see if I can convince you to do something about
>>> the first couple of items before I go any further.
>>>
>>> ------------------------------------------------------------------------------
>>> I really wish this had been broken up into separate issues with
>>> associated changesets, rather than one large combined changeset.
>>>
>>> I realize that breaking it up will be more work for you, but it would
>>> make a substantial difference in the quality of my review.
>>>
>> Yes, I thought about that, and the more work for myself is not the big concern here - it's more work for everyone. I am not a commitrer, so I would need a sponsor for very single change, and lastly each change requires vigorous testing, which takes a long while and uses JPRT/RBT resources. I would prefer if we made one last effort here and clear up this backlog if that's OK.
> Sponsoring a reviewed and well tested change is usually not a big deal
> for the sponsor; I'm happy to do it for you, especially now that we're
> both normally operating in the same repository. I understand the
> testing overhead, though different kinds of changes might require
> different levels of testing.
>
> That said, I went ahead and made my way through the existing webrev,
> with comments below. Your explanation of the rationale behind the
> string formatting changes was helpful; still difficult to review, but
> at least now I know why I'm working so hard :-)
>
> ------------------------------------------------------------------------------
>> - Passing values by value, not pointer, to constraint functions. It
>> was thought originally that me way want to “fix” the values, but it
>> was decided to be out of scope for the JEP. (requested by Coleen)
> I keep waffling about this, so I'll go ahead an mention it, even
> though I don't yet have a strong opinion either way.
>
> For the writer of a constraint function, not needing to deal with a
> pointer is clearly better.
>
> However, I actually kind of thought it was a feature that the values
> were passed by pointer reference rather than by value (though the
> pointer should be const-qualified if no "fixup" capability is
> intended). That prevents certain kinds of bugs due to implicit
> conversions; the types involved here provide pretty much all
> conversions between pairs of types. I'm also concerned about bugs
> resulting from reversal of the value and verbose arguments, especially
> since their order is not consistent throughout the internal APIs.
>
> There are two different pathways to these constraint functions, one of
> which is still using (and needs to use) pointers. This change
> dereferences these pointers, rather than taking the address of values
> on the other path. So which approach is used is a wash from the
> caller's perspective.
Made the "value,verbose" usage consistent in the APIs.
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>
> Pre-existing issue:
> It seems like there is a lot of very similar parameterized
> boiler-plate code here, and that using macros or templates could
> significantly reduce the amount of source code. Of course, it would
> be important to accomplish such a shortening without involving
> infrastructure that is itself overly complex, but I don't think
> anything particularly tricky should be needed here.
>
> This can be left for a later cleanup, or even ignored if nobody else
> is bothered by it.
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
> 66 static Flag::Error MaxPLABSizeBounds(const char* name, bool verbose, size_t value) {
> ...
> 71 "greater than ergonomic PLAB minimum size (" SIZE_FORMAT ")\n",
> 72 name, value, PLAB::min_size());
>
> In addition to the copy-paste error already mentioned for line 71, on
> line 72 PLAB::min_size() should be PLAB::max_size().
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
> 79 static Flag::Error MinMaxPLABSizeBounds(const char* name, bool verbose, size_t value) {
> 80 if (MinPLABSizeBounds(name, verbose, value) == Flag::SUCCESS) {
> 81 return MaxPLABSizeBounds(name, verbose, value);
> 82 }
> 83 return Flag::VIOLATES_CONSTRAINT;
> 84 }
>
> I think it would be more maintainable to capture the result of
> MinPLABSizeBounds in a "status" variable. If status is SUCCESS, set
> status to result of MaxPLABSizeBounds. Return status. This
> eliminates assumptions about possible MinPLABSizeBounds results.
>
> But see later comment about these helper functions.
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
> 53 static Flag::Error MinPLABSizeBounds(const char* name, bool verbose, size_t value) {
> 66 static Flag::Error MaxPLABSizeBounds(const char* name, bool verbose, size_t value) {
> 79 static Flag::Error MinMaxPLABSizeBounds(const char* name, bool verbose, size_t value) {
> 86 Flag::Error YoungPLABSizeConstraintFunc(bool verbose, size_t value) {
>
> Pre-existing issue:
> While I'm all for using helper functions to break things up into
> bite-sized chunks, in this case I think the helper functions are
> actually making the code longer and more complicated. A helper for
> the two calls to print_error_if_needed might be useful if this
> approach were taken.
>
> Please take this as a suggestion to look at, rather than a required
> change.
Sangheon said he will take a look at those.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
> 53 static Flag::Error MinPLABSizeBounds(const char* name, bool verbose, size_t value) {
> 54 #if INCLUDE_ALL_GCS
> 55 if ((UseConcMarkSweepGC || UseG1GC) && (value < PLAB::min_size())) {
> 56 print_error_if_needed(verbose,
> 57 "%s (" SIZE_FORMAT ") must be "
> 58 "greater than ergonomic PLAB minimum size (" SIZE_FORMAT ")\n",
>
> Pre-existing defect:
> Message says "greater than" but test is "greater or equal".
>
> The obvious fix to the previously mentioned problems in
> MaxPLABSizeBounds would have a similar problem.
Fixed.
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp
> src/share/vm/runtime/commandLineFlagRangeList.cpp
>
> Compiler and GC constraint functions consistently use error messages
> of the form:
>
> <optionname> (<optionvalue>) ...
>
> In this file the error messages are of the form
>
> <optionname>=<optionvalue>
>
> The range checking functions also use the second form.
>
> These should all be consistent.
>
> ------------------------------------------------------------------------------
Fixed.
cheers
More information about the hotspot-dev
mailing list