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