On 30/06/2016 11:13 PM, Yasumasa Suenaga wrote:
Hi Marcus,
Looks good, thanks for fixing this.
Thanks!
I'm waiting a second reviewer and accepting FC extension request.
Reviewed. This looks much neater. Thanks.
David
-----
Yasumasa
On 2016/06/30 22:10, Marcus Larsson wrote:
Hi
On 2016-06-30 15:01, Yasumasa Suenaga wrote:
Hi Marcus,
Can we keep the printing of the index # in LogConfiguration? That
would save us from passing it as a parameter to describe. (So, print
index, call describe, and then print newline.)
I've fixed it.
Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.07/
Looks good, thanks for fixing this.
Marcus
Thanks,
Yasumasa
On 2016/06/30 18:38, Marcus Larsson wrote:
Hi,
On 2016-06-30 11:31, Yasumasa Suenaga wrote:
Hi Marcus,
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.
Ah, I understood.
If we refactor that in this enhancement, we do not need to make
dynamic memory allocation.
I uploaded a new webrev.
I hope this webrev matches your suggestion :-)
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.06/
Looks good! Just a nit: Can we keep the printing of the index # in
LogConfiguration? That would save us from passing it as a parameter
to describe. (So, print index, call describe, and then print newline.)
Thanks,
Marcus
Thanks,
Yasumasa
On 2016/06/28 22:21, Yasumasa Suenaga wrote:
Hi Marcus,
I don't really like that we need to make dynamic allocations here.
Should use resource area? or char array?
If we should use char array, how long should we reserve for buffer?
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.
I think this is refactoring of LogOutput and LogConfiguration.
Now (after FC date), is this work accepted?
IMHO, refactoring is another enhancement from this.
If it is needed, I think this enhancement should be started after
refactoring.
If refactoring and this enhancement can be merged and be accepted,
I will start to work for it.
Thanks,
Yasumasa
On 2016/06/28 20:23, Marcus Larsson wrote:
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
