RFR: 8048093 - Explicitly setting := vs = in the -XX:+PrintFlagsFinal output
Vladimir Kozlov
vladimir.kozlov at oracle.com
Tue Jul 5 19:37:39 UTC 2016
On 7/5/16 12:09 PM, Jesper Wilhelmsson wrote:
> Den 5/7/16 kl. 20:56, skrev Vladimir Kozlov:
>> In CheckCompileThresholdScaling.java fix spacing for "double
>> CompileThresholdScaling" lines (it was wrong in original code too).
>
> Are you referring to the number of spaces between the flag and the '='?
yes
> It matches the output from PrintFlagsFinal. It looks skewed in the test
> because in the test the lines are aligned starting at the first
> character in the type name, but in the flag output it is aligned on the
> space between the type and the flag name:
>
> intx CompileThreshold = 10000
> double CompileThresholdScaling = 1.000000
Got it. Ignore my comment then. Changes are good.
Thanks,
Vladimir
>
> Please clarify if this was not what you meant.
>
>>
>> Otherwise looks good. No need for updated webrev.
>
> Thanks!
>
> /Jesper
>
>>
>> Thanks,
>> Vladimir
>>
>> On 6/29/16 9:55 AM, Jesper Wilhelmsson 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