PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Yasumasa Suenaga
yasuenag at gmail.com
Thu May 5 02:18:26 UTC 2016
On 2016/05/05 10:52, David Holmes wrote:
> +1 from me.
Thanks David!
> I will sponsor this - currently submitting to JPRT.
Marcus will be a sponsor.
Thanks,
Yasumasa
> Thanks,
> David
>
> On 4/05/2016 11:49 PM, Marcus Larsson wrote:
>> On 05/04/2016 03:43 PM, Yasumasa Suenaga wrote:
>>> Thanks, Marcus!
>>>
>>>> into something like
>>>>
>>>> if (!success || values > SIZE_MAX) {
>>>
>>>> Also, I think you probably need a static_cast here:
>>>>
>>>> 201 _rotate_size = value;
>>>
>>> I applied them to new webrev:
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.07/
>>
>> Looks good! Thanks for fixing this.
>>
>> Marcus
>>
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/05/04 21:38, Marcus Larsson wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 05/04/2016 02:29 PM, Yasumasa Suenaga wrote:
>>>>> Hi David, Marcus,
>>>>>
>>>>> Thank you for your comment.
>>>>> I uploaded new webrev. Could you review it again?
>>>>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.06/
>>>>
>>>> Can you join these two cases:
>>>>
>>>> 193 if (!success) {
>>>> 194 break;
>>>> 195 } else if (value > SIZE_MAX) {
>>>>
>>>>
>>>> into something like
>>>>
>>>> if (!success || values > SIZE_MAX) {
>>>>
>>>> so that we properly fail to configure the output and print the error
>>>> message when atojulong fails as well?
>>>>
>>>>
>>>> Also, I think you probably need a static_cast here:
>>>>
>>>> 201 _rotate_size = value;
>>>>
>>>> to avoid compiler warnings on 32-bit platforms.
>>>>
>>>> Thanks,
>>>> Marcus
>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2016/05/04 15:49, Marcus Larsson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 05/04/2016 03:45 AM, David Holmes wrote:
>>>>>>> On 4/05/2016 10:19 AM, Yasumasa Suenaga wrote:
>>>>>>>> Hi David, Marcus,
>>>>>>>>
>>>>>>>>> I would not have done that but instead used a temporary julong
>>>>>>>>> for the
>>>>>>>>> parse result, then range check, then assign to the actual
>>>>>>>>> _rotate_size
>>>>>>>>> etc with a cast.
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> However, I guess we will encounter error when we access to > 2GB
>>>>>>>> size file.
>>>>>>>> (Sorry, 4GB is incorrect.)
>>>>>>>
>>>>>>> That would seem to be a different issue.
>>>>>>>
>>>>>>>> I do not investigate for it yet. So I'm not sure whether it is
>>>>>>>> correct.
>>>>>>>> In terms of this issue, should I keep range check and use julong
>>>>>>>> temporary
>>>>>>>> value for _rotate_size?
>>>>>>>
>>>>>>> I would go this route yes. Use the Arguments code to do the
>>>>>>> parsing; apply the same range check as exists today (which may or
>>>>>>> may not be sufficient, but that is a different issue), then assign.
>>>>>>
>>>>>> I think that would be better too.
>>>>>>
>>>>>> Thanks,
>>>>>> Marcus
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2016/05/04 5:40, David Holmes wrote:
>>>>>>>>> On 3/05/2016 11: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!
>>>>>>>>>
>>>>>>>>> Yes good catch. I would have hoped the compiler would have
>>>>>>>>> complained
>>>>>>>>> about that on 32-bit.
>>>>>>>>>
>>>>>>>>>> I changed type of _rotate_size and _current_size to julong in new
>>>>>>>>>> webrev:
>>>>>>>>>
>>>>>>>>> I would not have done that but instead used a temporary julong
>>>>>>>>> for the
>>>>>>>>> parse result, then range check, then assign to the actual
>>>>>>>>> _rotate_size
>>>>>>>>> etc with a cast. That is less disruptive than changing the existing
>>>>>>>>> types IMHO.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.05/
>>>>>>>>>>
>>>>>>>>>>>> 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?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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