PING: RFR: JDK-8153074: UL: Show output option in VM.log jcmd

Tue Jun 28 11:23:36 UTC 2016

Hi,

On 06/28/2016 11:29 AM, Yasumasa Suenaga wrote:
> PING: Could you review and sponsor it?
>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.05/

I don't really like that we need to make dynamic allocations here. I 
would prefer to have the outputs be responsible for describing 
themselves just like David mentions. The current design of 
LogConfiguration::describe doesn't follow that pattern, but I really 
think it should. Therefore I suggest that we introduce a describe() 
function in LogOutput as part of this change, and move the code 
currently in LogConfiguration::describe to this function, adding the 
option text to it as well.

Thanks,
Marcus

>
> 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 
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>