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.
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
