RFR: 8048093 - Explicitly setting := vs = in the -XX:+PrintFlagsFinal output
Gerard Ziemski
gerard.ziemski at oracle.com
Fri Jul 8 17:07:15 UTC 2016
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.
>
>>
>>
>> # 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.
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:
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.
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