RFR(S): 8198608: Improvements to command-line flags printing
David Holmes
david.holmes at oracle.com
Thu Mar 15 04:39:23 UTC 2018
Hi Lutz,
On 14/03/2018 6:34 PM, Schmidt, Lutz wrote:
> Hi David,
> sorry this took so long. Gaining insight into what happened and getting rid of the effects was cumbersome. Here is what I achieved:
>
> The ResourceMark issue was embarrassingly easy to resolve. I just removed the "detour" via stringStream.
Ok.
> The failing test suffered from three facts:
> #1 (that's my "fault"): I changed the print layout (omitting whitespace) in cases when the expected field width is exceeded.
> #2 (test inherent): the regex used to parse each print_flag() line requires whitespace at places where they are not really needed.
> #3 (test inherent): Some tests contain an exact copy of what they expect as output as string literal. Those literals had to be adapted.
>
> I have adjusted the layout such that the regex matches ok. What remains is a "bad gutt feeling" because
> - the regex is very specific with respect to the print layout. Minor changes may cause it to break (again).
Not uncommon unfortunately.
> - the test, though being a jvmci test, verifies a locally assumed format, for all flags. Is that the right place?
> - I have no insight into why this verification is necessary. Is there another place relying on the verified format?
> - I regard relying on an exact char by char layout of the test output as sub-par testing style.
Yes not ideal.
> Anyway, there is a new webrev at http://cr.openjdk.java.net/~lucy/webrevs/8198608.01/
fill_to_pos can just be a static function in globals.cpp rather than
adding it to the Flags API.
I'm a little unclear on how the output will look before and after these
changes. The test change (which can only be seen in the patch file due
to webrev ignoring whitespace changes) looks odd:
"intx CompileThreshold = 1000
{pd product} {command line}",
"double CompileThresholdScaling = 1.000000
{product} {default}"
as I thought there would be better alignment e.g.
"intx CompileThreshold = 1000
{pd product} {command line}",
"double CompileThresholdScaling = 1.000000
{product} {default}"
Though I'm not sure the above is going to display correctly in the email.
Thanks,
David
> The bug can be found here: https://bugs.openjdk.java.net/browse/JDK-8198608
>
> It has undergone all testing we run @SAP. No issues were detected. May I please request you to have another look? Of course, anybody else is welcome to comment as well!
>
> Thank you,
> Lutz
>
> On 03.03.18, 18:04, "Schmidt, Lutz" <lutz.schmidt at sap.com> wrote:
>
> Hi David,
>
> I guess I have to apologize! There must be something wrong with my test coverage.
>
> As of now, I don't have an idea why a flag value would not be printed correctly.
>
> I will look into this, and the ResourceMark issue as well, asap.
>
> Regards,
> Lutz
>
> On 03.03.18, 10:30, "David Holmes" <david.holmes at oracle.com> wrote:
>
> There are also test failures:
>
> compiler/jvmci/compilerToVM/GetFlagValueTest
>
> java.lang.RuntimeException: Unexpected line in -XX:+PrintFlagsFinal
> output: bool BootstrapJVMCI = false {JVMCI experimental}{default}:
> expected true, was false
>
> Can you please ensure you're checked for all tests that use PrintFlags
> or PrintFlagsFinal and verify that they run okay in both product and
> fastdebug. I see the failures on multiple platforms but linux-x64 is one
> so you should be able to test that one easily.
>
> Thanks,
> David
>
> On 3/03/2018 2:44 PM, David Holmes wrote:
> > I'll sponsor this.
> >
> > I don't see Goetz's email to the list but will take it as per your
> > response.
> >
> > Cheers,
> > David
> >
> > On 3/03/2018 1:04 AM, Schmidt, Lutz wrote:
> >> Hi David,
> >>
> >> it would be great if you could sponsor this change. I was able to
> >> successfully test on darwinintel64, linuxs390x, linuxppc64, and
> >> linuxx86_64. Our AIX systems are not playing nice with me at the
> >> moment (no issues with the change, just general misbehavior).
> >>
> >> I have modified line 551 according to your suggestion (and line 519 as
> >> well). Webrev updated in-place.
> >>
> >> So let's hope for a second review over the weekend.
> >>
> >> Regards,
> >> Lutz
> >>
> >>
> >> On 02.03.18, 02:19, "David Holmes" <david.holmes at oracle.com> wrote:
> >>
> >> On 1/03/2018 11:25 PM, Schmidt, Lutz wrote:
> >> > Hi David,
> >> > thank you for looking at this. You are right, the comment is a
> >> useless leftover -> removed in if and else branch.
> >> Looks fine. Just need a second reviewer. Do you need a sponsor to
> >> test
> >> on additional platforms? Otherwise what platforms have you tested?
> >> > With the "\n" handling, I believe we are on the safe side. If a
> >> newline character is detected in the parameter string, it is replaced
> >> by a st->cr() call. That call does the expected on any platform, I
> >> would hope. Flag::print_as_flag() (not in the scope of the change)
> >> uses a similar handling.
> >> >
> >> > The newlines are contained in string literals in C code (e.g.
> >> default values for parameters) or stem from ccstrlist concatenations.
> >> That is all under VM control. So I do not see a risk here. You can try
> >> yourself on any platform with the -XX:DisableIntrinsic=test1 parameter
> >> multiple times.
> >> >
> >> > If a user manages to specify a parameter string with platform
> >> (windows) specific line terminators and hopes for correct (\n-like)
> >> handling, he or she will be disappointed. I would assume the
> >> PrintFlags formatting isn't the only place that's impacted.
> >> Sorry I mistakenly thought you had modified the newline handling,
> >> when
> >> you hadn't. If there is an issue it would be preexisting. I was
> >> wondering how you would get a multi-line ccstr value. If you
> >> entered it
> >> on the command-line e.g:
> >> java -XX:OnError="Line 1
> >> Line2"
> >> then I would expect to find the platform line separator within the
> >> string. In testing this with the existing PrintFlagsFinal Linux
> >> does:
> >> ccstrlist OnError = Line 1
> >> OnError += Line 2
> >> {product} {command line}
> >> but testing on Windows is a problem. The regular cmd shell can't
> >> take
> >> multi-line arguments. If you use the ^ escape trick it actually
> >> strips
> >> the newline and passes the arg as one line. So I guess the issue is
> >> somewhat moot. :)
> >> One further nit:
> >> 551 st->print("%s", "+=");
> >> should just be:
> >> 551 st->print("+=");
> >> Thanks,
> >> David
> >> > I have updated the webrev in-place with the comments removed.
> >> >
> >> > Thanks again, Lutz
> >> >
> >> >
> >> > On 28.02.18, 23:26, "David Holmes" <david.holmes at oracle.com>
> >> wrote:
> >> >
> >> > Hi Lutz,
> >> >
> >> > On 24/02/2018 2:48 AM, Schmidt, Lutz wrote:
> >> > > Dear all,
> >> > >
> >> > > may I please request reviews for this small enhancement:
> >> > >
> >> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8198608
> >> > > Webrev:
> >> http://cr.openjdk.java.net/~lucy/webrevs/8198608.00/
> >> > >
> >> > > The code in Flag::print_on() so far wasn’t very easy to
> >> understand. Changing the layout of what was printed required some deep
> >> thinking. I hope that, with my changes, future modifications will be
> >> easier.
> >> > >
> >> > > The before/after output of -XX:+PrintFlagsFinal is
> >> identical, except for those argument names which are longer than
> >> expected. In that case, the new version prints one space less, which
> >> is by intention.
> >> >
> >> > This all seems okay - and easier to modify further if needed.
> >> >
> >> > Two minor comments:
> >> >
> >> > 576 // Flag::print_on(...) redesign (!print_ranges)
> >> >
> >> > Isn't this the print_ranges case? But in any case not sure
> >> a comment
> >> > with "redesign" in it is that meaningful given you can't
> >> see the old design.
> >> >
> >> > Does the ccstr newline handling work on all platforms (ie
> >> Windows) - I'm
> >> > never sure when it suffices to check for '\n' and when we
> >> have to check
> >> > for the platform specific line terminators.
> >> >
> >> > Thanks,
> >> > David
> >> >
> >> > > Thank you!
> >> > > Lutz
> >> > >
> >> > >
> >> > >
> >> > > Dr. Lutz Schmidt | SAP JVM | PI SAP CP Core | T: +49
> >> (6227) 7-42834
> >> > >
> >> >
> >> >
> >>
>
>
>
>
More information about the hotspot-runtime-dev
mailing list