RFR(S): 8198608: Improvements to command-line flags printing
Schmidt, Lutz
lutz.schmidt at sap.com
Wed Mar 14 08:34:30 UTC 2018
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.
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).
- 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.
Anyway, there is a new webrev at http://cr.openjdk.java.net/~lucy/webrevs/8198608.01/
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