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,
David
> Thanks,
>
> Yasumasa
>
> 2016/06/13 13:24 "David Holmes" <david.holmes at oracle.com
> <mailto:david.holmes at 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
>
>
>
>
>
>
>
>
>