PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Marcus Larsson
marcus.larsson at oracle.com
Tue May 3 13:55:17 UTC 2016
On 05/03/2016 03:49 PM, Yasumasa Suenaga wrote:
>> 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/
That's good, but then we shouldn't limit it to SIZE_MAX. The overflow
check in logFileOutput.cpp should be removed.
>
>>> 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?
No I don't think that's a problem.
Thanks,
Marcus
>
>
> 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