PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g

Yasumasa Suenaga yasuenag at gmail.com
Wed May 4 00:19:36 UTC 2016


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.)

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?


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