PING: RFR: JDK-8153074: UL: Show output option in VM.log jcmd
David Holmes
david.holmes at oracle.com
Mon Jun 13 00:05:48 UTC 2016
Hi Yasumasa,
On 13/06/2016 9:30 AM, Yasumasa Suenaga wrote:
> Hi David,
>
> I think "config_string" is different from "option_string".
>
> -Xlog format (from -Xlog:help message):
> -Xlog[:[what][:[output][:[decorators][:output-options]]]]
>
> config_string: "what" (ex. gc=trace)
> option_string: "output-options" (ex. filecount=5)
>
> Currently, LogOutput handles tags and loglevels only as config_string.
> It does not contain output options.
Okay I'm starting to see the bigger picture here. In terms of the
overall logging configuration we might have, for example:
gc=trace -> stdout
runtime=info -> fileA
compiler=trace -> fileB
where the LHS is (part of) the configuration, and the RHS is the output.
So for each output we set its "configuration" to the associated LHS.
So options are a distinct property of outputs and so should have been a
first class entity in LogOutput all along.
Okay so looking at your v4 I have two comments:
First, hard-wiring OPTIONS_LEN. I don't know what the possible options
are so don't know if 100 is adequate.
Second, if the logging syntax is:
-Xlog[:[what][:[output][:[decorators][:output-options]]]]
then shouldn't the configuration be printed in the same order/format?
Thanks,
David
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/06/13 8:14, David Holmes wrote:
>> On 12/06/2016 11:10 PM, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>> Thank you for your comment.
>>>
>>>> Is there some reason the option string could not simply become part of
>>>> the existing configuration string?
>>>
>>> My first proposal keeps option string at LogOutput and its child class
>>> (See webrev.01).
>>> Marcus commented that option string should be generated from current
>>> configuration.
>>>
>>> I uploaded new webrev.
>>> Could you review again?
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.04/
>>
>> Sorry but I repeat my question - why is the option information not
>> simply part of the config_string?
>>
>> Thanks,
>> David
>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/06/12 6:44, David Holmes wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Sorry but this API seems poorly fitting to me. First
>>>> print_option_string seems the wrong name given that the base class,
>>>> LogOutput, has no notion of having an "option string". It seems to be
>>>> a supposedly generic "print other stuff" function that only one class
>>>> actually needs to implement.
>>>>
>>>> Secondly it inverts the style of the API used for everything else - we
>>>> have getters for all the other "properties" which are then printed by
>>>> the describe_current_configuration method. But this is instead a
>>>> "print" function where we ask the target to print itself. Mixing the
>>>> two styles seems messy. It probably would have been better to have had
>>>> a print-style API from the start - then adding the options would have
>>>> been a trivial extension for those output classes with options.
>>>>
>>>> In addition the change you made to describe_current_configuration is
>>>> not at all general purpose - you wanted a given format (print between
>>>> the config string and the decorators) for this one class and so you
>>>> added the code to support that format. But that format may not make
>>>> sense for other classes that might have "extra stuff" to print.
>>>>
>>>> Is there some reason the option string could not simply become part of
>>>> the existing configuration string? It seems to me that for a LogFile
>>>> these "options" really are part of the configuration.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> PS. The two hpp files would need their copyright years updated to
>>>> "2015, 2016,".
>>>>
>>>> On 11/06/2016 10:30 PM, Yasumasa Suenaga wrote:
>>>>> PING: Could you review it?
>>>>> We need a second reviewer.
>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.03/
>>>>>
>>>>> This change is small fix, and it helps us to confirm current
>>>>> FileLogOutput configuration.
>>>>> So I want to merge it to jdk 9.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2016/05/17 19:17, Yasumasa Suenaga wrote:
>>>>>> PING: Could you review it?
>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.03/
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2016/05/10 8:06, Yasumasa Suenaga wrote:
>>>>>>> We need a second reviewer.
>>>>>>> Could you review it?
>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.03/
>>>>>>>
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> On 2016/05/04 23:38, Marcus Larsson wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>
>>>>>>>> On 05/04/2016 04:12 PM, Yasumasa Suenaga wrote:
>>>>>>>>> Hi Marcus,
>>>>>>>>>
>>>>>>>>>> 93 out->print("filecount=%u,filesize=" SIZE_FORMAT "%s ",
>>>>>>>>>> _file_count, byte_size_in_proper_unit(_rotate_size),
>>>>>>>>>> proper_unit_for_byte_size(_rotate_size));
>>>>>>>>>
>>>>>>>>> Thanks, I applied it to new webrev:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.03/
>>>>>>>>
>>>>>>>> Looks OK.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Marcus
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Could you review again?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2016/05/04 22:35, Marcus Larsson wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 05/04/2016 02:59 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>> Hi Marcus,
>>>>>>>>>>>
>>>>>>>>>>> Thank you for your comment.
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.02/
>>>>>>>>>>
>>>>>>>>>> Looks better. The format for _rotate_size should be SIZE_FORMAT.
>>>>>>>>>>
>>>>>>>>>> While we're at it I think it would be good (as I mentioned) to
>>>>>>>>>> use
>>>>>>>>>> a proper unit for the filesize. Basically changing
>>>>>>>>>>
>>>>>>>>>> 93 out->print("filecount=%u,filesize=%lu ", _file_count,
>>>>>>>>>> _rotate_size);
>>>>>>>>>>
>>>>>>>>>> into
>>>>>>>>>>
>>>>>>>>>> 93 out->print("filecount=%u,filesize=" SIZE_FORMAT "%s ",
>>>>>>>>>> _file_count, byte_size_in_proper_unit(_rotate_size),
>>>>>>>>>> proper_unit_for_byte_size(_rotate_size));
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Marcus
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I fixed to use _rotate_size and _file_count directly to show
>>>>>>>>>>> VM.log list jcmd.
>>>>>>>>>>> I do not store option string, and I added new function to print
>>>>>>>>>>> option string.
>>>>>>>>>>>
>>>>>>>>>>> Could you review it again?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> Yasumasa
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2016/05/04 18:33, Marcus Larsson wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 05/03/2016 01:43 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>> PING: Could you review and sponsor it?
>>>>>>>>>>>>>
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.01/
>>>>>>>>>>>>
>>>>>>>>>>>> I would prefer to generate the option string from the actual
>>>>>>>>>>>> options rather than saving the string from when it was
>>>>>>>>>>>> configured. This would also produce/print the options for
>>>>>>>>>>>> outputs that are using the defaults (which is not the case
>>>>>>>>>>>> now).
>>>>>>>>>>>> The filesize option could then use byte_size_in_proper_unit and
>>>>>>>>>>>> proper_unit_for_byte_size to make it easier to read.
>>>>>>>>>>>>
>>>>>>>>>>>> Also, get_option_string() should just be called
>>>>>>>>>>>> option_string().
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Marcus
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> This patch makes to show option string of LogFileOutput.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2016/04/19 22:55, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>> I adapted changes to jdk9/hs/hotspot repos.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.01/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Please review.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 2016/04/18 23:09, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>> PING:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I've sent review request for JDK-8153074.
>>>>>>>>>>>>>>> Could you review it?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.00/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> If this patch is merged, user can confirm output option via
>>>>>>>>>>>>>>> VM.log jcmd.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Please review and sponsor it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2016/04/11 18:29, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>> PING: Could you review and sponsor it?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.00/
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 2016/03/31 22:35, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> CC'ed to serviceability-dev.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Could you review it?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.00/
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 2016/03/30 23:09, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> This request review is related to [1].
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I want to see output option (filecount, filesize) in
>>>>>>>>>>>>>>>>>> VM.log jcmd.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Output sample:
>>>>>>>>>>>>>>>>>> #2: gc.log gc=trace,
>>>>>>>>>>>>>>>>>> filecount=5,filesize=1048576
>>>>>>>>>>>>>>>>>> time,level,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I uploaded webrev. Could you review it?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.00/
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-March/018704.html
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
More information about the hotspot-runtime-dev
mailing list