RFR(S): 8198608: Improvements to command-line flags printing
Schmidt, Lutz
lutz.schmidt at sap.com
Fri Mar 16 08:34:40 UTC 2018
Thank you, David!
And yes, I'm aware of Gerard's testing efforts.
Regards,
Lutz
On 16.03.18, 02:32, "David Holmes" <david.holmes at oracle.com> wrote:
Seems fine to me. As you should have seen Gerard has been running
additional tests, so I'll leave it to him now.
Thanks,
David
On 16/03/2018 1:48 AM, Schmidt, Lutz wrote:
> Hi David, Mikael,
>
> I agree, fill_to_pos() now is a local static function.
>
> You are not alone with your confusion about how the output would look like, given the "misaligned" string literals in the source code. There is an easy explanation, though:
>
> The different output columns are individually aligned. Some are left-justified, others right-justified. You find the details as comments in the source code. For example, column 1, the data type, is right-justified. To show correct alignment in the source code, you would have to shift the string literal "double ..." two positions to the left. With that, the "=" and the "...} {...“ sequences align perfectly. The only change I made to the string literals was adding one blank character after the numeric value.
>
> As per request by Mikael Vidstedt, I have added, as comment, a few output lines to the source code. That might improve imagination of what is printed when looking at or even modifying the code.
>
> To avoid confusion, I have uploaded a new webrev at http://cr.openjdk.java.net/~lucy/webrevs/8198608.02/
>
> Best Regards,
> Lutz
>
>
> On 15.03.18, 05:39, "David Holmes" <david.holmes at oracle.com> wrote:
>
> 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