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

Sun Jun 12 13:10:58 UTC 2016

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/

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