RFR: 8048093 - Explicitly setting := vs = in the -XX:+PrintFlagsFinal output
Gerard Ziemski
gerard.ziemski at oracle.com
Mon Jul 11 14:06:04 UTC 2016
Looks good - (r)eviewed.
cheers
> On Jul 11, 2016, at 5:56 AM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>
> 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