PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
David Holmes
david.holmes at oracle.com
Wed May 4 01:45:52 UTC 2016
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.
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