PING: RFR: JDK-8153074: UL: Show output option in VM.log jcmd
Yasumasa Suenaga
yasuenag at gmail.com
Thu Jun 30 09:31:50 UTC 2016
Hi Marcus,
>> 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.
Ah, I understood.
If we refactor that in this enhancement, we do not need to make dynamic memory allocation.
I uploaded a new webrev.
I hope this webrev matches your suggestion :-)
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.06/
Thanks,
Yasumasa
On 2016/06/28 22:21, Yasumasa Suenaga wrote:
> 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