PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Marcus Larsson
marcus.larsson at oracle.com
Tue May 3 12:41:33 UTC 2016
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