PING: RFR: JDK-8153074: UL: Show output option in VM.log jcmd
David Holmes
david.holmes at oracle.com
Mon Jun 13 04:24:00 UTC 2016
Hi Yasumasa,
On 13/06/2016 1:45 PM, Yasumasa Suenaga wrote:
> Hi David,
>
> Thank you for your comment.
>
>> So options are a distinct property of outputs and so should have been
>> a first class entity in LogOutput all along.
>
> I agree to you.
> But I think we need to discuss about it with logging folks.
>
> I uploaded a new webrev. It removes fixed buffer length and changes the
> order of output.
> Could you review again?
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.05/
It's okay to wait and hear what opinions others may have before
changing things based on my comments. :) The fixed buffer may be okay -
as I said I don't know what the potential options are, so don't know if
it is okay or not.
Using dynamic allocation avoids that but raises other concerns - like
calling vm_exit_on_out_of_memory on failure; or whether to use malloc or
resource area?
Lets wait for other feedback before going further.
Thanks,
David
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/06/13 9:05, David Holmes wrote:
>> 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 serviceability-dev
mailing list