PING: Could you review it? BTW, should I add jdk9-fc-request label to JBS?
Thanks, Yasumasa 2016/06/13 13:24 "David Holmes" <david.hol...@oracle.com>: > 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 >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>