PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Marcus Larsson
marcus.larsson at oracle.com
Wed May 4 13:49:44 UTC 2016
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