RFR: 8048093 - Explicitly setting := vs = in the -XX:+PrintFlagsFinal output
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Fri Jul 8 02:09:54 UTC 2016
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.
>
>
> # 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);
}
>
> 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;
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