PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
David Holmes
david.holmes at oracle.com
Thu May 5 07:42:42 UTC 2016
On 5/05/2016 12:18 PM, Yasumasa Suenaga wrote:
>
> On 2016/05/05 10:52, David Holmes wrote:
>> +1 from me.
>
> Thanks David!
>
>
>> I will sponsor this - currently submitting to JPRT.
>
> Marcus will be a sponsor.
Sorry too late already done.
David
>
> Thanks,
>
> Yasumasa
>
>
>> 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