PING: RFR: JDK-8153074: UL: Show output option in VM.log jcmd
David Holmes
david.holmes at oracle.com
Sun Jun 12 23:14:08 UTC 2016
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