PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g

[Yasumasa Suenaga]

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/

> 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,

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
>>>>>>>
>>>>>>