PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
David Holmes
david.holmes at oracle.com
Thu May 5 01:52:58 UTC 2016
+1 from me.
I will sponsor this - currently submitting to JPRT.
Thanks,
David
On 4/05/2016 11:49 PM, Marcus Larsson wrote:
> On 05/04/2016 03:43 PM, Yasumasa Suenaga wrote:
>> Thanks, Marcus!
>>
>>> into something like
>>>
>>> if (!success || values > SIZE_MAX) {
>>
>>> Also, I think you probably need a static_cast here:
>>>
>>> 201 _rotate_size = value;
>>
>> I applied them to new webrev:
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.07/
>
> Looks good! Thanks for fixing this.
>
> Marcus
>
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2016/05/04 21:38, Marcus Larsson wrote:
>>> Hi,
>>>
>>>
>>> On 05/04/2016 02:29 PM, Yasumasa Suenaga wrote:
>>>> Hi David, Marcus,
>>>>
>>>> Thank you for your comment.
>>>> I uploaded new webrev. Could you review it again?
>>>>
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.06/
>>>
>>> Can you join these two cases:
>>>
>>> 193 if (!success) {
>>> 194 break;
>>> 195 } else if (value > SIZE_MAX) {
>>>
>>>
>>> into something like
>>>
>>> if (!success || values > SIZE_MAX) {
>>>
>>> so that we properly fail to configure the output and print the error
>>> message when atojulong fails as well?
>>>
>>>
>>> Also, I think you probably need a static_cast here:
>>>
>>> 201 _rotate_size = value;
>>>
>>> to avoid compiler warnings on 32-bit platforms.
>>>
>>> Thanks,
>>> Marcus
>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2016/05/04 15:49, Marcus Larsson wrote:
>>>>> Hi,
>>>>>
>>>>> On 05/04/2016 03:45 AM, David Holmes wrote:
>>>>>> On 4/05/2016 10:19 AM, Yasumasa Suenaga wrote:
>>>>>>> Hi David, Marcus,
>>>>>>>
>>>>>>>> I would not have done that but instead used a temporary julong
>>>>>>>> for the
>>>>>>>> parse result, then range check, then assign to the actual
>>>>>>>> _rotate_size
>>>>>>>> etc with a cast.
>>>>>>>
>>>>>>> Thanks.
>>>>>>> However, I guess we will encounter error when we access to > 2GB
>>>>>>> size file.
>>>>>>> (Sorry, 4GB is incorrect.)
>>>>>>
>>>>>> That would seem to be a different issue.
>>>>>>
>>>>>>> I do not investigate for it yet. So I'm not sure whether it is
>>>>>>> correct.
>>>>>>> In terms of this issue, should I keep range check and use julong
>>>>>>> temporary
>>>>>>> value for _rotate_size?
>>>>>>
>>>>>> I would go this route yes. Use the Arguments code to do the
>>>>>> parsing; apply the same range check as exists today (which may or
>>>>>> may not be sufficient, but that is a different issue), then assign.
>>>>>
>>>>> I think that would be better too.
>>>>>
>>>>> Thanks,
>>>>> Marcus
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> On 2016/05/04 5:40, David Holmes wrote:
>>>>>>>> On 3/05/2016 11:49 PM, Yasumasa Suenaga wrote:
>>>>>>>>>> This seems broken to me, we're passing &_rotate_size (a
>>>>>>>>>> size_t*) to
>>>>>>>>>> atojulong() which takes a julong*. If sizeof(julong) >
>>>>>>>>>> sizeof(size_t)
>>>>>>>>>> then that's a problem, no?
>>>>>>>>>
>>>>>>>>> Thanks, Marcus!
>>>>>>>>
>>>>>>>> Yes good catch. I would have hoped the compiler would have
>>>>>>>> complained
>>>>>>>> about that on 32-bit.
>>>>>>>>
>>>>>>>>> I changed type of _rotate_size and _current_size to julong in new
>>>>>>>>> webrev:
>>>>>>>>
>>>>>>>> I would not have done that but instead used a temporary julong
>>>>>>>> for the
>>>>>>>> parse result, then range check, then assign to the actual
>>>>>>>> _rotate_size
>>>>>>>> etc with a cast. That is less disruptive than changing the existing
>>>>>>>> types IMHO.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.05/
>>>>>>>>>
>>>>>>>>>>> the question for the UL owners is whether the change in
>>>>>>>>>>> semantics is
>>>>>>>>>>> appropriate: previously the filesize was interpreted as a
>>>>>>>>>>> value in
>>>>>>>>>>> KB, whereas now, without a suffix, it is just bytes.
>>>>>>>>>
>>>>>>>>> Is it no problem?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2016/05/03 21:41, Marcus Larsson wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 05/03/2016 02:25 PM, David Holmes wrote:
>>>>>>>>>>> Adding in the runtime team as they now own UL.
>>>>>>>>>>>
>>>>>>>>>>> I've reviewed the change from a coding perspective - the
>>>>>>>>>>> question for
>>>>>>>>>>> the UL owners is whether the change in semantics is appropriate:
>>>>>>>>>>> previously the filesize was interpreted as a value in KB,
>>>>>>>>>>> whereas
>>>>>>>>>>> now, without a suffix, it is just bytes.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> David
>>>>>>>>>>>
>>>>>>>>>>> On 3/05/2016 9:44 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>>> PING: Could you review and sponsor it?
>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.04/
>>>>>>>>>>
>>>>>>>>>> This seems broken to me, we're passing &_rotate_size (a
>>>>>>>>>> size_t*) to
>>>>>>>>>> atojulong() which takes a julong*. If sizeof(julong) >
>>>>>>>>>> sizeof(size_t)
>>>>>>>>>> then that's a problem, no?
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Marcus
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2016/04/21 18:37, Yasumasa Suenaga wrote:
>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>
>>>>>>>>>>>>>> So it just registered with me that currently filesize is
>>>>>>>>>>>>>> interpreted
>>>>>>>>>>>>>> as a value in KB. With this change it will be in bytes -
>>>>>>>>>>>>>> that means
>>>>>>>>>>>>>> tests will need fixing eg:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> hotspot/test/serviceability/logging/TestLogRotation.java
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>> I've fixed it in new webrev:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.04/
>>>>>>>>>>>>>
>>>>>>>>>>>>> Following jtreg tests are passed:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - hotspot/test/gc/logging
>>>>>>>>>>>>> - hotspot/test/runtime/logging
>>>>>>>>>>>>> - hotspot/test/serviceability/logging
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2016/04/21 14:43, David Holmes wrote:
>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 20/04/2016 7:15 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ... on 32-bit size_t and julong are not the same size so
>>>>>>>>>>>>>>>> we would
>>>>>>>>>>>>>>>> still need to ensure we don't specify a filesize that is
>>>>>>>>>>>>>>>> greater
>>>>>>>>>>>>>>>> than
>>>>>>>>>>>>>>>> SIZE_MAX on 32-bit.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Oh... I understood.
>>>>>>>>>>>>>>> I've fixed and uploaded new webrev. Could you review again?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.03/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So it just registered with me that currently filesize is
>>>>>>>>>>>>>> interpreted
>>>>>>>>>>>>>> as a value in KB. With this change it will be in bytes -
>>>>>>>>>>>>>> that means
>>>>>>>>>>>>>> tests will need fixing eg:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> hotspot/test/serviceability/logging/TestLogRotation.java
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> That change in semantics may not be desirable, but I'll leave
>>>>>>>>>>>>>> that to
>>>>>>>>>>>>>> the owners of this code to decide (and I hope they jump in
>>>>>>>>>>>>>> soon!)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I note that in the existing range check:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> if (value == SIZE_MAX || value > SIZE_MAX / K) {
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> the first clause is redundant. So your change seems okay.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> David
>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2016/04/20 15:04, David Holmes wrote:
>>>>>>>>>>>>>>>> On 20/04/2016 3:25 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> > You still removed the size bounds checks:
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> > 190 size_t value = parse_value(value_str);
>>>>>>>>>>>>>>>>> > 191 if (value == SIZE_MAX || value > SIZE_MAX
>>>>>>>>>>>>>>>>> / K) {
>>>>>>>>>>>>>>>>> > 192 errstream->print_cr("Invalid option: %s must
>>>>>>>>>>>>>>>>> be in
>>>>>>>>>>>>>>>>> range
>>>>>>>>>>>>>>>>> [0, "
>>>>>>>>>>>>>>>>> > 193 SIZE_FORMAT "]",
>>>>>>>>>>>>>>>>> FileSizeOptionKey,
>>>>>>>>>>>>>>>>> SIZE_MAX / K);
>>>>>>>>>>>>>>>>> > 194 success = false;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> SIZE_MAX is defined as ULONG_MAX in stdint.h [1].
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Ah I hadn't realized this was an external value, I
>>>>>>>>>>>>>>>> thought it
>>>>>>>>>>>>>>>> was some
>>>>>>>>>>>>>>>> internally enforced maximum file size limit. So this is
>>>>>>>>>>>>>>>> just an
>>>>>>>>>>>>>>>> overflow check really, and ...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Arguments::atojulong(atomull) checks value range [2].
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ... we already do an overflow check in here, but ...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thus I do not think we do not need to check value range in
>>>>>>>>>>>>>>>>> LogFileOutput::parse_options().
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ... on 32-bit size_t and julong are not the same size so
>>>>>>>>>>>>>>>> we would
>>>>>>>>>>>>>>>> still need to ensure we don't specify a filesize that is
>>>>>>>>>>>>>>>> greater
>>>>>>>>>>>>>>>> than
>>>>>>>>>>>>>>>> SIZE_MAX on 32-bit.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> > Thanks, I had missed that example usage buried in
>>>>>>>>>>>>>>>>> there,
>>>>>>>>>>>>>>>>> but am
>>>>>>>>>>>>>>>>> still
>>>>>>>>>>>>>>>>> > surprised none of these "options" for the handling the
>>>>>>>>>>>>>>>>> file are
>>>>>>>>>>>>>>>>> > explicitly documented.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I do not know how we can documented about it.
>>>>>>>>>>>>>>>>> (Is it internal process in Oracle?)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> No I just meant that amongst all that help text you already
>>>>>>>>>>>>>>>> modified,
>>>>>>>>>>>>>>>> there is nothing, that I could see, that actually
>>>>>>>>>>>>>>>> describes the
>>>>>>>>>>>>>>>> possible options for filesize.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I can help for it if I can
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>> https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/generic/stdint.h;h=442762728b899aa8ec219299692fce5953d796b0;hb=HEAD#l259
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>> http://hg.openjdk.java.net/jdk9/hs/hotspot/file/8005261869c9/src/share/vm/runtime/arguments.cpp#l804
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 2016/04/20 11:24 "David Holmes" <david.holmes at oracle.com
>>>>>>>>>>>>>>>>> <mailto:david.holmes at oracle.com>>:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 19/04/2016 11:50 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> > Hi David,
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> > Thank you for your comment.
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> > I uploaded new webrev. Could you review again?
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.02/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> You still removed the size bounds checks:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 190 size_t value = parse_value(value_str);
>>>>>>>>>>>>>>>>> 191 if (value == SIZE_MAX || value >
>>>>>>>>>>>>>>>>> SIZE_MAX / K) {
>>>>>>>>>>>>>>>>> 192 errstream->print_cr("Invalid option: %s must
>>>>>>>>>>>>>>>>> be in
>>>>>>>>>>>>>>>>> range [0, "
>>>>>>>>>>>>>>>>> 193 SIZE_FORMAT "]",
>>>>>>>>>>>>>>>>> FileSizeOptionKey,
>>>>>>>>>>>>>>>>> SIZE_MAX / K);
>>>>>>>>>>>>>>>>> 194 success = false;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> >> Which makes me wonder if atomull needs renaming -
>>>>>>>>>>>>>>>>> does the
>>>>>>>>>>>>>>>>> "m" mean
>>>>>>>>>>>>>>>>> >> memory? atojulong would seem more appropriate
>>>>>>>>>>>>>>>>> regardless.
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> > I renamed to atojulong() in new webrev.
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> >> Not directly related to your change but I was
>>>>>>>>>>>>>>>>> surprised
>>>>>>>>>>>>>>>>> that the
>>>>>>>>>>>>>>>>> >> various log file options don't seem to be
>>>>>>>>>>>>>>>>> documented
>>>>>>>>>>>>>>>>> anywhere
>>>>>>>>>>>>>>>>> in the
>>>>>>>>>>>>>>>>> >> -Xlog:help output.
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> > I updated help message in new webrev.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks, I had missed that example usage buried in
>>>>>>>>>>>>>>>>> there,
>>>>>>>>>>>>>>>>> but am
>>>>>>>>>>>>>>>>> still
>>>>>>>>>>>>>>>>> surprised none of these "options" for the handling
>>>>>>>>>>>>>>>>> the file
>>>>>>>>>>>>>>>>> are
>>>>>>>>>>>>>>>>> explicitly documented.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> > Thanks,
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> > Yasumasa
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>> > On 2016/04/19 10:14, David Holmes wrote:
>>>>>>>>>>>>>>>>> >> Hi Yasumasa,
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> On 19/04/2016 12:06 AM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> >>> PING:
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>> I've sent review request for JDK-8153073.
>>>>>>>>>>>>>>>>> >>> Could you review it?
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>> If this patch is merged, user can set logfile
>>>>>>>>>>>>>>>>> size with
>>>>>>>>>>>>>>>>> k/m/g.
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> Your webrev seems out of date with respect to the
>>>>>>>>>>>>>>>>> current
>>>>>>>>>>>>>>>>> code - the
>>>>>>>>>>>>>>>>> >> logfile size processing is done in
>>>>>>>>>>>>>>>>> LogFileOutput::parse_options not
>>>>>>>>>>>>>>>>> >> configure_rotation. And of course you now need
>>>>>>>>>>>>>>>>> to work
>>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>>> jdk9/hs not
>>>>>>>>>>>>>>>>> >> hs-rt.
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> That aside:
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> src/share/vm/runtime/arguments.cpp
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> I don't think you need to add the Arguments::
>>>>>>>>>>>>>>>>> to the
>>>>>>>>>>>>>>>>> atomull
>>>>>>>>>>>>>>>>> calls when
>>>>>>>>>>>>>>>>> >> you are executing in Arguments code - lines
>>>>>>>>>>>>>>>>> 2643, 2660
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> This comment could be updated to delete "memory"
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> 788 // Parses a memory size specification
>>>>>>>>>>>>>>>>> string.
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> Which makes me wonder if atomull needs renaming -
>>>>>>>>>>>>>>>>> does the
>>>>>>>>>>>>>>>>> "m" mean
>>>>>>>>>>>>>>>>> >> memory? atojulong would seem more appropriate
>>>>>>>>>>>>>>>>> regardless.
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> ---
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> src/share/vm/logging/logFileOutput.cpp
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> You removed the size checking logic.
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> Not directly related to your change but I was
>>>>>>>>>>>>>>>>> surprised
>>>>>>>>>>>>>>>>> that the
>>>>>>>>>>>>>>>>> >> various log file options don't seem to be
>>>>>>>>>>>>>>>>> documented
>>>>>>>>>>>>>>>>> anywhere
>>>>>>>>>>>>>>>>> in the
>>>>>>>>>>>>>>>>> >> -Xlog:help output.
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >> Thanks,
>>>>>>>>>>>>>>>>> >> David
>>>>>>>>>>>>>>>>> >> -----
>>>>>>>>>>>>>>>>> >>
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>> Please review it.
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>> Thanks,
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>> Yasumasa
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>>
>>>>>>>>>>>>>>>>> >>> On 2016/04/11 18:28, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> >>>> PING: Could you review it?
>>>>>>>>>>>>>>>>> >>>> We need more reviewer.
>>>>>>>>>>>>>>>>> >>>>
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> >>>>
>>>>>>>>>>>>>>>>> >>>>
>>>>>>>>>>>>>>>>> >>>> Thanks,
>>>>>>>>>>>>>>>>> >>>>
>>>>>>>>>>>>>>>>> >>>> Yasumasa
>>>>>>>>>>>>>>>>> >>>>
>>>>>>>>>>>>>>>>> >>>>
>>>>>>>>>>>>>>>>> >>>> On 2016/03/31 22:33, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> >>>>> CC'ed to serviceability-dev.
>>>>>>>>>>>>>>>>> >>>>>
>>>>>>>>>>>>>>>>> >>>>> Could you review it?
>>>>>>>>>>>>>>>>> >>>>>
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>
>>>>>>>>>>>>>>>>> >>>>>
>>>>>>>>>>>>>>>>> >>>>> Thanks,
>>>>>>>>>>>>>>>>> >>>>>
>>>>>>>>>>>>>>>>> >>>>> Yasumasa
>>>>>>>>>>>>>>>>> >>>>>
>>>>>>>>>>>>>>>>> >>>>>
>>>>>>>>>>>>>>>>> >>>>> On 2016/03/31 18:24, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> >>>>>> Hi Marcus,
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>>> You're missing an include of arguments.hpp in
>>>>>>>>>>>>>>>>> logFileOutput.cpp.
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>> arguments.hpp is included in
>>>>>>>>>>>>>>>>> precompiled.hpp . So
>>>>>>>>>>>>>>>>> build was
>>>>>>>>>>>>>>>>> succeeded.
>>>>>>>>>>>>>>>>> >>>>>> However, it should be included in
>>>>>>>>>>>>>>>>> logFileOutput.cpp .
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>> I uploaded a new webrev. Could you review
>>>>>>>>>>>>>>>>> again?
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>> Thanks,
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>> Yasumasa
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>>
>>>>>>>>>>>>>>>>> >>>>>> On 2016/03/31 16:48, Marcus Larsson wrote:
>>>>>>>>>>>>>>>>> >>>>>>> Hi,
>>>>>>>>>>>>>>>>> >>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>> On 03/30/2016 04:09 PM, Yasumasa Suenaga
>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>> >>>>>>>> Hi all,
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>> This request review is related to [1].
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>> I want to set filesize option with k/m/g as
>>>>>>>>>>>>>>>>> below:
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> -Xlog:gc=trace:file=gc.log:time:filecount=5,filesize=10m
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>> Memory size option (e.g. -Xmx) can be set with
>>>>>>>>>>>>>>>>> k/m/g .
>>>>>>>>>>>>>>>>> >>>>>>>> I think we can use option parser in
>>>>>>>>>>>>>>>>> arguments.cpp .
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>> I uploaded webrev. Could you review it?
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.00/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>> You're missing an include of arguments.hpp in
>>>>>>>>>>>>>>>>> logFileOutput.cpp.
>>>>>>>>>>>>>>>>> >>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>> Apart from that, this looks good to me.
>>>>>>>>>>>>>>>>> >>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>> Thanks,
>>>>>>>>>>>>>>>>> >>>>>>> Marcus
>>>>>>>>>>>>>>>>> >>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>>
>>>>>>>>>>>>>>>>> >>>>>>>> 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