RFR: 8048093 - Explicitly setting := vs = in the -XX:+PrintFlagsFinal output
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Mon Jul 11 16:37:46 UTC 2016
Thanks Gerard!
/Jesper
Den 11/7/16 kl. 16:06, skrev Gerard Ziemski:
> 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