PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Yasumasa Suenaga
yasuenag at gmail.com
Thu Apr 21 09:37:00 UTC 2016
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 serviceability-dev
mailing list