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