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