RFR: 8048093 - Explicitly setting := vs = in the -XX:+PrintFlagsFinal output
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Mon Jul 11 10:56:51 UTC 2016
Hi Gerard,
Den 8/7/16 kl. 19:07, skrev Gerard Ziemski:
> hi Jesper,
>
>> On Jul 7, 2016, at 9:09 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>
>> Hi Gerard,
>>
>> Thanks for reviewing!
>>
>> Den 6/7/16 kl. 19:17, skrev Gerard Ziemski:
>>> hi Jesper,
>>>
>>> I really like the new output - very good job, but have a few feedbacks regarding the implemention:
>>>
>>> # 1
>>>
>>> This point is just my opinion:
>>>
>>> In general, whouldn't it have been easier, from the implementation point of view, to add a new "bool" field tracking whether the "command line" was overriden?
>>>
>>> Tacking the "original command line" bit, though clever, in the Flag.Flags enum seems to have complicated the code handling that field quite a bit with numerous modifications of existing code required.
>>
>> I guess that is a matter of taste. :)
>> Unless you have a strong objection to this style I'd prefer to keep it the way it is.
>
> Fair enough. I’d have done things differently, but at least your way we don’t use any more memory.
Yes, that's one of my main motivations to do it this way.
>>> # 2
>>>
>>> 1 void Flag::set_origin(Flags origin) {
>>> 2 assert((origin & VALUE_ORIGIN_MASK) == origin, "sanity");
>>> 3 origin = (origin == COMMAND_LINE) ? Flags(origin | ORIG_COMMAND_LINE) : origin;
>>> 4 _flags = Flags((_flags & ~VALUE_ORIGIN_MASK) | origin);
>>> 5 }
>>>
>>> I don't like that the assert on line 2 - it is immediately invalidated on line 3.
>>
>> Right, this is not optimal. The problem is that the input variable is reused as a temporary. This is more clear:
>>
>> void Flag::set_origin(Flags origin) {
>> assert((origin & VALUE_ORIGIN_MASK) == origin, "sanity");
>> Flags new_origin = Flags((origin == COMMAND_LINE) ? Flags(origin | ORIG_COMMAND_LINE) : origin);
>> _flags = Flags((_flags & ~VALUE_ORIGIN_MASK) | new_origin);
>> }
>
> That helps, thanks.
>
>
>>
>>>
>>> With an addition of "bool" field as I pointed out in feedback #1, this wouldn't be an issue.
>>>
>>>
>>> # 3
>>>
>>> The names of functions don't quite match what we are doing, ex:
>>>
>>> bool Flag::is_command_line()
>>>
>>> should be perhaps renamed as:
>>>
>>> bool Flag::is_orig_command_line() ?
>>>
>>> Similar for "Flag::set_command_line()" and "CommandLineFlagsEx::setOnCmdLine()"
>>>
>>> Here we're not using the "COMMAND_LINE" value, but "ORIG_COMMAND_LINE" instead.
>>
>> There is no practical difference between is_command_line and is_orig_command_line. The old meaning of is_command_line does not exist anymore, once a flag has been set on the command line it will always be set on the command line. So I don't see any increased value in using a longer name.
>> The fact that we use a different flag to see where the flag originated is an implementation detail that doesn't need to leak outside of is/set_command_line() imho.
>>
>>>
>>>
>>> # 4
>>>
>>> Trivial issue - shouldn't the assert:
>>>
>>> // Size - 2 here because we want to fit } and \0 in there as well.
>>> assert(buffer_used < (buffer_size - 2), "Too small buffer");
>>>
>>> be applied right before:
>>>
>>> jio_snprintf(kind + buffer_used, buffer_size - buffer_used, "}");
>>>
>>> ie, taken outside the "for (int i = 0; data[i].flag != -1; i++)" loop?
>>
>> The intention was to catch a too small buffer already in the loop. If we continue to write outside the buffer and the assert is outside the loop we might crash before reaching the assert. I see now that my version can exhibit the same behavior since we could actually write outside the buffer before the assert here as well. I changed it to:
>>
>> size_t length = strlen(d.name);
>> // Size - 2 here because we want to fit } and \0 in there as well.
>> assert(buffer_used - length < (buffer_size - 2), "Too small buffer");
>> jio_snprintf(kind + buffer_used, buffer_size - buffer_used, "%s", d.name);
>> buffer_used += length;
>
> Looking at the assert more, I think you got the sign wrong, ie. instead of
>
> assert(buffer_used - length < (buffer_size - 2), "Too small buffer");
>
> I think it should be:
>
> assert(buffer_used + length < (buffer_size - 2), "Too small buffer”);
>
> or I have been staring at the code for too long.
You are right. Good catch!
>
> Also, I disagree about that assert that checks whether “}\0” fits. I think assert should be single purpose and check one thing at the time - the way it’s written right now it checks whether the “d.name” AND “}\0” both fit. I believe the code should be more something like:
I'm fine with that. It definitely makes the code easier to read.
>
> if ((_flags & KIND_MASK) != 0) {
> bool is_first = true;
> const size_t buffer_size = 64;
> size_t buffer_used = 1;
> char kind[buffer_size] = “{";
>
> for (int i = 0; data[i].flag != -1; i++) {
> Data d = data[i];
> if ((_flags & d.flag) != 0) {
> if (is_first) {
> is_first = false;
> } else {
> assert(buffer_used + 1 < buffer_size, "Too small buffer");
> jio_snprintf(kind + buffer_used, buffer_size - buffer_used, " ");
> buffer_used++;
> }
> size_t length = strlen(d.name);
> assert(buffer_used + length < buffer_size, "Too small buffer");
> jio_snprintf(kind + buffer_used, buffer_size - buffer_used, "%s", d.name);
> buffer_used += length;
> }
> }
> assert(buffer_used + 2 < buffer_size, "Too small buffer");
> jio_snprintf(kind + buffer_used, buffer_size - buffer_used, "}”);
>
> st->print("%20s", kind);
> }
>
> Lastly, probably another personal preference, but I prefer “buffer_used+2 < buffer_size” over “buffer_used < buffer_size-2” since “buffer_used” is the one advancing and “buffer_size" is constant.
Agreed.
I have made the changes suggested with a small modification to the last assert.
I use <= instead of < in the last assert since it would be OK to actually fill
the entire size of the buffer here.
New version:
http://cr.openjdk.java.net/~jwilhelm/8048093/webrev.04/
Increment:
http://cr.openjdk.java.net/~jwilhelm/8048093/webrev.04-incremental/
Thanks,
/Jesper
>
>
> cheers
>
>>
>> Are you OK with this version?
>>
>> New webrev is here:
>> http://cr.openjdk.java.net/~jwilhelm/8048093/webrev.03/
>>
>> Diff to previous version:
>> http://cr.openjdk.java.net/~jwilhelm/8048093/webrev.03-incremental/
>>
>> Thanks,
>> /Jesper
>>
>>>
>>>
>>> cheers
>>>
>>>> On Jun 29, 2016, at 11:55 AM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>>>
>>>> After some more testing I made a few smaller updates:
>>>>
>>>> * Using jio_snprintf instead of snprintf in Flag::print_kind_and_origin()
>>>> Looking at other code, it seemed to be the right thing to do.
>>>>
>>>> * Using size_t instead of int for the size and used counter in Flag::print_kind_and_origin()
>>>> Windows compiler complained.
>>>>
>>>> * ORIG_COMMAND_LINE = 1 << 19
>>>>
>>>> * Fixed some tests that was surprised by the new output from PrintFlagsFinal.
>>>>
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~jwilhelm/8048093/webrev.02/
>>>>
>>>> Thanks,
>>>> /Jesper
>>>>
>>>>
>>>> Den 23/6/16 kl. 20:54, skrev Jesper Wilhelmsson:
>>>>> Den 23/6/16 kl. 20:22, skrev Vladimir Kozlov:
>>>>>> why you skipped 19?:
>>>>>>
>>>>>> ORIG_COMMAND_LINE = 1 << 20,
>>>>>
>>>>> No good reason really, just leaving some space since ORIG_COMMAND_LINE is not a
>>>>> kind. I can change it to 19 if you think that's better.
>>>>>
>>>>>>
>>>>>> Otherwise looks good.
>>>>>
>>>>> Thanks for reviewing!
>>>>>
>>>>> /Jesper
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 6/23/16 8:04 AM, Jesper Wilhelmsson wrote:
>>>>>>> Den 22/6/16 kl. 21:33, skrev Coleen Phillimore:
>>>>>>>
>>>>>>>>
>>>>>>>> I agree with Vladimir. There's no way I'm going to remember that a colon
>>>>>>>> mean that the value was changed.
>>>>>>> The colon is what we have today to indicate that a value was changed :)
>>>>>>>
>>>>>>> I made a different solution in which I explicitly print the origin with words.
>>>>>>> To make it look better I changed the
>>>>>>> printing routine somewhat. The origin is currently at the end of the line.
>>>>>>> This version will also print other origins like config file, environment, and
>>>>>>> management as suggested by Dan.
>>>>>>>
>>>>>>> New webrev:
>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8048093/webrev.01/
>>>>>>>
>>>>>>> Examples of -XX:+PrintFlagsFinal with and without the change:
>>>>>>>
>>>>>>> This is the new version:
>>>>>>>
>>>>>>> ccstr AbortVMOnExceptionMessage =
>>>>>>> {diagnostic} {default}
>>>>>>> uintx AdaptiveSizeDecrementScaleFactor =
>>>>>>> 4 {product} {default}
>>>>>>> intx AliasLevel =
>>>>>>> 3 {C2 product} {default}
>>>>>>> intx CMSAbortablePrecleanWaitMillis =
>>>>>>> 100 {manageable} {default}
>>>>>>> bool CMSClassUnloadingEnabled =
>>>>>>> false {product} {command line}
>>>>>>> intx ConditionalMoveLimit =
>>>>>>> 3 {C2 pd product} {default}
>>>>>>> size_t InitialHeapSize =
>>>>>>> 134217728 {product} {ergonomic}
>>>>>>> size_t MaxMetaspaceSize =
>>>>>>> 18446744073709547520 {product} {default}
>>>>>>> size_t NewSize =
>>>>>>> 1966080 {product} {command line,
>>>>>>> ergonomic}
>>>>>>> ccstrlist OnError = {product} {default}
>>>>>>> intx PSAdaptiveSizePolicyResizeVirtualSpaceAlot =
>>>>>>> -1 {develop} {default}
>>>>>>> bool UseAdaptiveGenerationSizePolicyAtMajorCollection =
>>>>>>> true {product} {default}
>>>>>>> bool UseCISCSpill =
>>>>>>> true {C2 pd develop} {default}
>>>>>>> bool UseCLMUL =
>>>>>>> true {ARCH product} {default}
>>>>>>> bool UseCompressedOops =
>>>>>>> true {lp64_product} {ergonomic}
>>>>>>> bool UseConcMarkSweepGC =
>>>>>>> true {product} {command line}
>>>>>>>
>>>>>>> This is the old (current) version:
>>>>>>>
>>>>>>> ccstr AbortVMOnExceptionMessage =
>>>>>>> {diagnostic}
>>>>>>> uintx AdaptiveSizeDecrementScaleFactor =
>>>>>>> 4 {product}
>>>>>>> intx AliasLevel =
>>>>>>> 3 {C2 product}
>>>>>>> intx CMSAbortablePrecleanWaitMillis =
>>>>>>> 100 {manageable}
>>>>>>> bool CMSClassUnloadingEnabled :=
>>>>>>> false {product}
>>>>>>> intx ConditionalMoveLimit =
>>>>>>> 3 {C2 pd product}
>>>>>>> size_t InitialHeapSize :=
>>>>>>> 134217728 {product}
>>>>>>> size_t MaxMetaspaceSize =
>>>>>>> 18446744073709547520 {product}
>>>>>>> size_t NewSize :=
>>>>>>> 1966080 {product}
>>>>>>> ccstrlist OnError = {product}
>>>>>>> intx PSAdaptiveSizePolicyResizeVirtualSpaceAlot =
>>>>>>> -1 {develop}
>>>>>>> bool UseAdaptiveGenerationSizePolicyAtMajorCollection =
>>>>>>> true {product}
>>>>>>> bool UseCISCSpill =
>>>>>>> true {C2 pd develop}
>>>>>>> bool UseCompressedOops :=
>>>>>>> true {lp64_product}
>>>>>>> bool UseConcMarkSweepGC :=
>>>>>>> true {product}
>>>>>>>
>>>>>>> /Jesper
>>>>>>>
>>>>>>>> Coleen
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/21/16 4:15 PM, Vladimir Kozlov wrote:
>>>>>>>>> Nobody will remember the meaning of such marking. You should use normal words.
>>>>>>>>> The output already have flag's type as, for example, "{C2 product}". You can
>>>>>>>>> add an other word there to indicate type
>>>>>>>>> of initialization: default|command|ergo.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> On 6/21/16 12:09 PM, Jesper Wilhelmsson wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Please review this change to make -XX:+PrintFlagsFinal distinguish between
>>>>>>>>>> flags changed on the command line and flags
>>>>>>>>>> changed by the ergonomics.
>>>>>>>>>>
>>>>>>>>>> To enable us to remember if a flag was set on the command line or not after
>>>>>>>>>> having been changed by the ergonomics I use
>>>>>>>>>> another bit in the Flag struct.
>>>>>>>>>>
>>>>>>>>>> The actual symbols printed by PrintFlagsFinal are chosen somewhat arbitrary
>>>>>>>>>> and I'm open to suggestions (bike sheding)
>>>>>>>>>> about how to visualize this the best.
>>>>>>>>>>
>>>>>>>>>> The old decorations are:
>>>>>>>>>>
>>>>>>>>>> ' =' - Default value (no decoration)
>>>>>>>>>> ':=' - Value was changed, either by command line or ergonomics or other
>>>>>>>>>>
>>>>>>>>>> The new decorations are:
>>>>>>>>>>
>>>>>>>>>> ' =' - Default value (no decoration)
>>>>>>>>>> ':=' - Set on the command line
>>>>>>>>>> '?=' - Changed by the ergonomics
>>>>>>>>>> '!=' - Set on the command line AND changed by the ergonomics
>>>>>>>>>> '-=' - Any other origin, like changed by management API or taken from a
>>>>>>>>>> config file
>>>>>>>>>>
>>>>>>>>>> My reasoning behind selecting these characters are that ':=' looks like a
>>>>>>>>>> solid assignment and will therefore represent
>>>>>>>>>> the command line. You never know what you get from the ergonomics, so '?='
>>>>>>>>>> seems appropriate. '!=' because you'd
>>>>>>>>>> want to
>>>>>>>>>> know that the ergonomics changed a value you had set on the command line.
>>>>>>>>>>
>>>>>>>>>> Another option could be to use a colon ':=' for the command line, and a
>>>>>>>>>> comma ',=' for the ergonomics. That would
>>>>>>>>>> naturally give us the semi-colon ';=' for something set on the command line
>>>>>>>>>> and changed by the ergonomics. I didn't go
>>>>>>>>>> with this because the colon and semi-colon can be hard to distinguish from
>>>>>>>>>> each other.
>>>>>>>>>>
>>>>>>>>>> As mentioned above there are other origins, the enum contains eight values.
>>>>>>>>>> There is no mention of the other types in
>>>>>>>>>> the bug and there are no is_...() for the other types in globals.cpp, so
>>>>>>>>>> they didn't seem as important. If the general
>>>>>>>>>> opinion is that these other origins are as important, or we might as
>>>>>>>>>> well..., I'd suggest that we use letters as
>>>>>>>>>> decorations. Let me know if you have an opinion.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8048093
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8048093/webrev.00/
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> /Jesper
>
More information about the hotspot-dev
mailing list