PING: RFR: JDK-8153074: UL: Show output option in VM.log jcmd
Yasumasa Suenaga
yasuenag at gmail.com
Wed Jun 22 03:53:35 UTC 2016
2016/06/22 9:48 "David Holmes" <david.holmes at oracle.com>:
>
> 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! I added.
Yasumasa
> 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
>>
>>
>>
>>
>>
>>
>>
>>
>>
More information about the hotspot-runtime-dev
mailing list