PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Yasumasa Suenaga
yasuenag at gmail.com
Tue May 3 13:49:00 UTC 2016
> 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!
I changed type of _rotate_size and _current_size to julong in new webrev:
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