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
