PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Yasumasa Suenaga
yasuenag at gmail.com
Tue May 3 11:44:22 UTC 2016
PING: Could you review and sponsor it?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.04/
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 serviceability-dev
mailing list