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