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
