PING: RFR: JDK-8153074: UL: Show output option in VM.log jcmd
Yasumasa Suenaga
yasuenag at gmail.com
Tue Jun 28 13:21:14 UTC 2016
Hi Marcus,
> I don't really like that we need to make dynamic allocations here.
Should use resource area? or char array?
If we should use char array, how long should we reserve for buffer?
> 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.
I think this is refactoring of LogOutput and LogConfiguration.
Now (after FC date), is this work accepted?
IMHO, refactoring is another enhancement from this.
If it is needed, I think this enhancement should be started after
refactoring.
If refactoring and this enhancement can be merged and be accepted,
I will start to work for it.
Thanks,
Yasumasa
On 2016/06/28 20:23, Marcus Larsson wrote:
> 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
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>
More information about the serviceability-dev
mailing list