PING: RFR: JDK-8153074: UL: Show output option in VM.log jcmd
Yasumasa Suenaga
yasuenag at gmail.com
Tue Jun 28 09:29:55 UTC 2016
PING: Could you review and sponsor it?
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.05/
I've requested FC extension for this.
Thanks,
Yasumasa
On 2016/06/13 13:24, David Holmes wrote:
> 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 hotspot-runtime-dev
mailing list