PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
David Holmes
david.holmes at oracle.com
Tue May 3 12:25:32 UTC 2016
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/
>
>
> 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