2016/06/22 9:48 "David Holmes" <david.hol...@oracle.com>: > > On 22/06/2016 9:37 AM, Yasumasa Suenaga wrote: >> >> PING: Could you review it? >> BTW, should I add jdk9-fc-request label to JBS? > > > Yes this enhancement will need approval. Please add the label and other information as outlined here: > > http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004443.html
Thanks! I added. Yasumasa > Thanks, > David > > > >> Thanks, >> >> Yasumasa >> >> 2016/06/13 13:24 "David Holmes" <david.hol...@oracle.com >> <mailto: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 >> >> >> >> >> >> >> >> >>
