[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: JDK-7090324: gclog rotation via external tool
From: Yasumasa Suenaga <yasu () ysfactory ! dip ! jp>
Date: 2014-03-06 0:04:15
Message-ID: 5317BB7F.6000903 () ysfactory ! dip ! jp
[Download RAW message or body]
Hi,
Thank you so much!
I'm glad to contribute this new feature.
BTW, could you backport this feature to JDK7 and 8?
I usually use JDK7 for my work.
I want to this feature on JDK7.
Thanks,
Yasumasa
On 03/06/2014 01:14 AM, Yumin Qi wrote:
> Yasumasa,
>
> This looks good. I will sponsor the integration --- since it adds
> new feature to jcmd, need internal process approval so after it is
> approved, I will do the push.
>
> Thanks
> Yumin
>
> On 3/5/2014 12:32 AM, Yasumasa Suenaga wrote:
>> Hi Erik, Yumin,
>>
>> Thank you for comments.
>>
>> I've updated brief of GCLogFileSize.
>> New webrev is here:
>> http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.07/
>>
>> Could you review again?
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>> On 03/05/2014 03:47 PM, Erik Helin wrote:
>>> Hi Yasu,
>>>
>>> I think this looks good except for the comment in globals.hpp for
>>> GCLogFileSize which needs to be updated.
>>>
>>> What do you think about the following?
>>>
>>> product(uintx, GCLogFileSize, 8*K, \
>>> "GC log file size, requires UseGCLogFileRotation. " \
>>> "Set to 0 to only trigger rotation via jcmd") \
>>>
>>> Thanks,
>>> Erik
>>>
>>> On 03/05/2014 04:52 AM, Yasumasa Suenaga wrote:
>>>> Hi Yumin,
>>>>
>>>> Thank you for comments.
>>>> I've updated webrev:
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.06/
>>>>
>>>> Could you review this?
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 03/04/2014 03:29 PM, Yumin Qi wrote:
>>>>> Yasumasa,
>>>>>
>>>>> Seems no comments for this email so far. I have some comments of
>>>>> the changes:
>>>>>
>>>>> 1) arguments.cpp:
>>>>>
>>>>> a) line 1896 & 1897, please merge these two lines into one, the
>>>>> length will not exceeds longest lines in this file.
>>>>> b) line 1898 - 1990, move line back 4 spaces, in hotspot we use
>>>>> two space indentation. The original version is not right.
>>>>>
>>>>> 2) diagnosticCommand.cpp:
>>>>> line 665, 672 is empty, can be removed. See other function
>>>>> formats.
>>>>>
>>>>> 3) diagnosticCommand.hpp:
>>>>> remove one extra empty line above class RotateGCDcmd
>>>>> The empty lines inside the class, between functions can be
>>>>> removed too. This style changed the former style since class
>>>>> JMXStartRemoteDCmd added.
>>>>>
>>>>> 4) TestGCLogRotationViaJcmd.java:
>>>>> Java code default indentation is 4 spaces.
>>>>>
>>>>> 5) ostream.hpp:
>>>>> I would like to add some comments to how to use 'force' like:
>>>>> /*true, force log file rotation from outside JVM */
>>>>>
>>>>> Thanks
>>>>> Yumin
>>>>>
>>>>> On 2/12/2014 6:32 AM, Yasumasa Suenaga wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I've uploaded new webrev:
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.05/
>>>>>>
>>>>>> Erik pointed me that my patch changes current behavior for
>>>>>> GCLogFileSize.
>>>>>>
>>>>>> In current implementation, default value of GCLogFileSize is set to
>>>>>> "0" and
>>>>>> if user set this value to less than 8K, JVM adjust it to 8K.
>>>>>>
>>>>>>
>>>>>> Below are the scenarios:
>>>>>>
>>>>>> 1. -Xloggc:test.log -XX:+UseGCLogFileRotation
>>>>>> -XX:NumberOfGCLogFiles=3
>>>>>> Should result in GCLogFileSize "0" (GC log rotation will be
>>>>>> turned off)
>>>>>>
>>>>>> 2. -Xloggc:test.log -XX:+UseGCLogFileRotation
>>>>>> -XX:NumberOfGCLogFiles=3 -XX:GCLogFileSize=10K
>>>>>> Should result in GCLogFileSize 10K
>>>>>>
>>>>>> 3. -Xloggc:test.log -XX:+UseGCLogFileRotation
>>>>>> -XX:NumberOfGCLogFiles=3 -XX:GCLogFileSize=2K
>>>>>> Should result in GCLogFileSize 8K
>>>>>>
>>>>>> From the result of 3, we can think that GCLogFileSize is set to
>>>>>> 8K by
>>>>>> default.
>>>>>> So I want to change default value of this to 8K in globals.hpp .
>>>>>>
>>>>>> And I want to treat "0" as special number for rotating by external
>>>>>> trigger.
>>>>>> From the result of 1, if GCLogFileSIze is set to "0",
>>>>>> UseGCLogFileRotation is set to false.
>>>>>> Definition of GCLogFileSize in globals.hpp, "0" means "no
>>>>>> rotation" .
>>>>>> Thus I think this changes does not make different behavior from
>>>>>> current implementation.
>>>>>> ------------------------
>>>>>> product(uintx, GCLogFileSize,
>>>>>> 0, \
>>>>>> "GC log file size (default: 0 bytes, no rotation).
>>>>>> " \
>>>>>> "It requires
>>>>>> UseGCLogFileRotation") \
>>>>>> ------------------------
>>>>>>
>>>>>>
>>>>>> Could you review this ?
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 02/05/2014 09:13 PM, Yasumasa Suenaga wrote:
>>>>>>> Sorry, I forgot to paste URL of new webrev :-P
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.04/
>>>>>>>
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>> On 02/05/2014 09:09 PM, Yasumasa Suenaga wrote:
>>>>>>>> Hi Erik,
>>>>>>>>
>>>>>>>> Thank you for reviewing again!
>>>>>>>> I've updated new webrev.
>>>>>>>>
>>>>>>>> On 02/05/2014 07:40 PM, Erik Helin wrote:
>>>>>>>>> Hi Yasumasa,
>>>>>>>>>
>>>>>>>>> I've looked through the latest patch, it is much better! I just
>>>>>>>>> have two comments:
>>>>>>>>>
>>>>>>>>> - ostream.hpp:
>>>>>>>>> Why did you add GCLogFileSize != 0 in should_rotate? The old
>>>>>>>>> check
>>>>>>>>> just checked that _bytes_written > GCLogFileSize.
>>>>>>>>
>>>>>>>> Default value of GCLogFileSIze is "0" in globals.hpp .
>>>>>>>> So if this state is missed, should_rotate() returns true in
>>>>>>>> anytime.
>>>>>>>>
>>>>>>>>
>>>>>>>>> - TestGCLogRotationViaJcmd.java:
>>>>>>>>> Could you use the helper class JDKToolLauncher to start
>>>>>>>>> jmap? The
>>>>>>>>> code would then be slightly easier to read:
>>>>>>>>>
>>>>>>>>> for (int times = 1; times < NUM_LOGS; times++) {
>>>>>>>>> // Run jcmd <pid> GC.rotate_log
>>>>>>>>> JDKToolLauncher jmap = JDKToolLauncher.create("jmap")
>>>>>>>>> .addToolArg(pid)
>>>>>>>>> .addToolArg("GC.rotate_log");
>>>>>>>>> ProcessBuilder pb = new ProcessBuilder(jmap.getCommand());
>>>>>>>>>
>>>>>>>>> // Make sure we didn't crash
>>>>>>>>> OutputAnalyzer output = new OutputAnalyzer(pb.start());
>>>>>>>>> output.shouldHaveExitValue(0);
>>>>>>>>> }
>>>>>>>>
>>>>>>>> I've fixed. Could you check the patch?
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Erik
>>>>>>>>>
>>>>>>>>> On 01/30/2014 12:12 PM, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi Staffan,
>>>>>>>>>>
>>>>>>>>>> I've uploaded new webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.03/
>>>>>>>>>>
>>>>>>>>>> On 2014/01/30 17:23, Staffan Larsen wrote:
>>>>>>>>>>> Would it be possible for the Diagnostic Command to output the
>>>>>>>>>>> location
>>>>>>>>>>> of the rotated log? When invoking the command it would be good
>>>>>>>>>>> to get
>>>>>>>>>>> some kind of feedback.
>>>>>>>>>>
>>>>>>>>>> I changed rotate_log() to redirect messages to jcmd.
>>>>>>>>>> If GC.rotate_log is executed, we can get messages on jcmd
>>>>>>>>>> console
>>>>>>>>>> as below:
>>>>>>>>>> ------------
>>>>>>>>>> $ jcmd 18976 GC.rotate_log
>>>>>>>>>> 18976:
>>>>>>>>>> 2014-01-30 19:59:39 GC log rotation request has been received.
>>>>>>>>>> Saved as
>>>>>>>>>> test.log.0
>>>>>>>>>> 2014-01-30 19:59:39 GC log file created test.log.1
>>>>>>>>>> ------------
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> test/gc/7090324/Test7090324.java:
>>>>>>>>>>> - I think this needs to have the Oracle copyright notice as
>>>>>>>>>>> well.
>>>>>>>>>>> - Tests should now use descriptive names, not bug numbers:
>>>>>>>>>>> https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> - nits: lots of missing spaces before ‘{‘, and after ‘for’,
>>>>>>>>>>> ‘if’
>>>>>>>>>>> - line 47: you don’t need to clean up old files, jtreg will
>>>>>>>>>>> give
>>>>>>>>>>> you a
>>>>>>>>>>> fresh scratch directory to run in
>>>>>>>>>>
>>>>>>>>>> I've fixed.
>>>>>>>>>> Could you review again?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>> /Staffan
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 30 jan 2014, at 08:08, Yasumasa
>>>>>>>>>>> Suenaga<suenaga.yasumasa@lab.ntt.co.jp> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Erik, Staffan,
>>>>>>>>>>>>
>>>>>>>>>>>> I've uploaded new webrev. Could you review this ?
>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.02/
>>>>>>>>>>>>
>>>>>>>>>>>> This patch includes fixes from comments of Staffan and Erik.
>>>>>>>>>>>>
>>>>>>>>>>>> And I created new test of this patch as Test7090324 .
>>>>>>>>>>>> This test works fine with jtreg.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>> On 2014/01/30 0:55, Yasumasa Suenaga wrote:
>>>>>>>>>>>>> Hi Erik,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2014/01/30 0:13, Erik Helin wrote:
>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> (have to use HTML email to get a width of more than 78
>>>>>>>>>>>>>> chars,
>>>>>>>>>>>>>> sorry)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> why did you change the code in arguments.cpp in the method
>>>>>>>>>>>>>> check_gc_log_consistency?
>>>>>>>>>>>>>
>>>>>>>>>>>>> In current implementation, check_gclog_consistency()
>>>>>>>>>>>>> checks three
>>>>>>>>>>>>> parameters:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - GC log filename
>>>>>>>>>>>>> - NumberOfGCLogFiles
>>>>>>>>>>>>> - GCLogFileSize
>>>>>>>>>>>>>
>>>>>>>>>>>>> My customer uses external trigger "ONLY" for rotating logs.
>>>>>>>>>>>>> If they want to do that, GCLogFileSize does not need.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Next, the gcLogFileStream::rotate_log method now does a lot
>>>>>>>>>>>>>> of things.
>>>>>>>>>>>>>> Could you separate out the first block into a new method,
>>>>>>>>>>>>>> gcLogFileStream::should_rotate(bool force)?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This was, the code would read:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> bool gcLogFileStream::should_rotate(bool force) {
>>>>>>>>>>>>>>> return force || _bytes_writen>= GCLogFileSize;
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> void gcLogFileStream::rotate_log(bool force) {
>>>>>>>>>>>>>>> char time_msg[FILENAMEBUFLEN];
>>>>>>>>>>>>>>> char time_str[EXTRACHARLEN];
>>>>>>>>>>>>>>> char current_file_name[FILENAMEBUFLEN];
>>>>>>>>>>>>>>> char renamed_file_name[FILENAMEBUFLEN];
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> if (!should_rotate(force)) {
>>>>>>>>>>>>>>> return;
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Could you please update your patch?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I will do that.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> There is a new empty line in the rotate_log method:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> #ifdef ASSERT
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> could you please remove it?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I will do that.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> The logging change in rotate_log uses a different kind of
>>>>>>>>>>>>>> if/else
>>>>>>>>>>>>>> syntax
>>>>>>>>>>>>>> than the rest of the file:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> if (force) {
>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> else {
>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The other if/else statements in the file uses:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> if (cond) {
>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>> } else {
>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Could you please update your change to use the same if/else
>>>>>>>>>>>>>> syntax?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I will do that.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> This part of the change duplicates the code:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> + jio_snprintf(time_msg, sizeof(time_msg), "%s GC log
>>>>>>>>>>>>>> rotation
>>>>>>>>>>>>>> request has been received. Saved as %s\n",
>>>>>>>>>>>>>> + os::local_time_string((char *)time_str, sizeof(time_str)),
>>>>>>>>>>>>>> + renamed_file_name);
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> + else {
>>>>>>>>>>>>>> + jio_snprintf(time_msg, sizeof(time_msg), "%s GC log
>>>>>>>>>>>>>> file has
>>>>>>>>>>>>>> reached the"
>>>>>>>>>>>>>> " maximum size. Saved as %s\n",
>>>>>>>>>>>>>> - os::local_time_string((char *)time_str, sizeof(time_str)),
>>>>>>>>>>>>>> + os::local_time_string((char *)time_str, sizeof(time_str)),
>>>>>>>>>>>>>> renamed_file_name);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Could you instead just change the message, as in:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> const char* msg = forced ? "%s GC log rotation request
>>>>>>>>>>>>>>> has been
>>>>>>>>>>>>>>> received. Saved as %s\n" :
>>>>>>>>>>>>>>> "%s GC log file has reached the maximum size. Saved as
>>>>>>>>>>>>>>> %s\n";
>>>>>>>>>>>>>>> jio_snprintf(msg, os::local_time_string((char *)time_str,
>>>>>>>>>>>>>>> sizeof(time_str)), renamed_file_name);
>>>>>>>>>>>>>
>>>>>>>>>>>>> I will do that.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> The declaration of rotate_log in ostream.hpp still uses
>>>>>>>>>>>>>> the old
>>>>>>>>>>>>>> variable name is_force, it should use force,
>>>>>>>>>>>>>> just as the definition.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Sorry, I will fix it.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Finally, could you add a test that tests your change? Have a
>>>>>>>>>>>>>> look
>>>>>>>>>>>>>> at the other tests
>>>>>>>>>>>>>> in hotspot/test/gc to see how you can do it
>>>>>>>>>>>>>> (you might want to use some functionality from
>>>>>>>>>>>>>> hotspot/test/testlibrary).
>>>>>>>>>>>>>
>>>>>>>>>>>>> I found three tests as following:
>>>>>>>>>>>>>
>>>>>>>>>>>>> [ysuenaga@xelvis test]$ find . -iname "*jcmd*"
>>>>>>>>>>>>> ./runtime/NMT/JcmdWithNMTDisabled.java
>>>>>>>>>>>>> ./runtime/NMT/JcmdScale.java
>>>>>>>>>>>>> ./gc/TestG1ZeroPGCTJcmdThreadPrint.java
>>>>>>>>>>>>>
>>>>>>>>>>>>> I understand that these tests checks output
>>>>>>>>>>>>> (stdout/stderr) with
>>>>>>>>>>>>> OutputAnalyzer.
>>>>>>>>>>>>> However, my patch affects target VM. So I guess current test
>>>>>>>>>>>>> cannot
>>>>>>>>>>>>> check
>>>>>>>>>>>>> that GC log rotation is succeeded.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Should I make test which checks exit value of jcmd ?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Erik
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 2014-01-29 15:28, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>> Hi Staffan,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thank you for reviewing!
>>>>>>>>>>>>>>> I've uploaded new webrev.
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.01/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2014/01/29 20:56, Staffan Larsen wrote:
>>>>>>>>>>>>>>>> Yasumasa,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> src/share/vm/runtime/arguments.cpp
>>>>>>>>>>>>>>>> no comments
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> src/share/vm/runtime/safepoint.cpp
>>>>>>>>>>>>>>>> I was surprised that gc log size was checked after each
>>>>>>>>>>>>>>>> safe
>>>>>>>>>>>>>>>> point. That seems an uneccssary burden to place on a safe
>>>>>>>>>>>>>>>> point.
>>>>>>>>>>>>>>>> Instead we should switch to a periodic task that checks
>>>>>>>>>>>>>>>> the gc
>>>>>>>>>>>>>>>> log size. However, this is unrelated to you patch, so
>>>>>>>>>>>>>>>> please
>>>>>>>>>>>>>>>> ignore for now.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Agree.
>>>>>>>>>>>>>>> However, I think that PeriodicTask also is not appropriate
>>>>>>>>>>>>>>> for this.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Size of GC log file is increased when GC is occurred.
>>>>>>>>>>>>>>> So I think rotate function should be called at entry of
>>>>>>>>>>>>>>> each GC
>>>>>>>>>>>>>>> events
>>>>>>>>>>>>>>> e.g. VM_GC_Operation::doit_prologue() etc...
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> src/share/vm/runtime/vm_operations.hpp
>>>>>>>>>>>>>>>> line 402: nit: missing space before {
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Fixed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> line 405: I think ‘force’ is a better name than ‘is_force’
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I removed "force" option from DCmd.
>>>>>>>>>>>>>>> So I removed this from VMOperation.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> src/share/vm/services/diagnosticCommand.cpp
>>>>>>>>>>>>>>>> line 666: What does this do without the -force option? It
>>>>>>>>>>>>>>>> looks
>>>>>>>>>>>>>>>> to me that the non-force case will happen after each safe
>>>>>>>>>>>>>>>> point
>>>>>>>>>>>>>>>> (see above) and that there is no need to ever do this
>>>>>>>>>>>>>>>> from a
>>>>>>>>>>>>>>>> diagnostic command. Can we remove the option?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Indeed.
>>>>>>>>>>>>>>> I removed "force" option.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> line 677: “Target VM does not support GC log file
>>>>>>>>>>>>>>>> rotation."
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Fixed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> nits: some missing spaces before ‘{' and after ‘if'
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Fixed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> src/share/vm/services/diagnosticCommand.hpp
>>>>>>>>>>>>>>>> I think RotateGCLogDCmd should require the “control”
>>>>>>>>>>>>>>>> permission
>>>>>>>>>>>>>>>> when executed via JMX, so please add:
>>>>>>>>>>>>>>>> static const JavaPermission permission() {
>>>>>>>>>>>>>>>> JavaPermission p =
>>>>>>>>>>>>>>>> {"java.lang.management.ManagementPermission",
>>>>>>>>>>>>>>>> "control", NULL};
>>>>>>>>>>>>>>>> return p;
>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Added.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> line 394: Maybe “Force the GC log file to be rotated.”
>>>>>>>>>>>>>>>> is a
>>>>>>>>>>>>>>>> better description?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Fixed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> src/share/vm/utilities/ostream.cpp
>>>>>>>>>>>>>>>> line 662: I think ‘force’ is a better name than ‘is_force’
>>>>>>>>>>>>>>>> line 668: The comment says exactly the same thing as the
>>>>>>>>>>>>>>>> code so
>>>>>>>>>>>>>>>> I think it can be skipped
>>>>>>>>>>>>>>>> line 671: “GC log file rotation occurs by external trigger
>>>>>>>>>>>>>>>> ONLY."
>>>>>>>>>>>>>>>> line 675: "not need” -> “no need”
>>>>>>>>>>>>>>>> line 718: "GC log rotation request has been received”
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Fixed them.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> src/share/vm/utilities/ostream.hpp
>>>>>>>>>>>>>>>> no comments
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> /Staffan
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 24 jan 2014, at 14:50, Yasumasa
>>>>>>>>>>>>>>>> Suenaga<yasu@ysfactory.dip.jp> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I've created webrev:
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.00/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This patch works fine on current jdk9/hs-rt .
>>>>>>>>>>>>>>>>> Could you review this?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I am just an Author. So I need a sponsor.
>>>>>>>>>>>>>>>>> Could you help me?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Please cooperate.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 2013/12/06 0:05, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Did someone read my email?
>>>>>>>>>>>>>>>>>> I really hope to merge "JDK-7090324: gclog rotation via
>>>>>>>>>>>>>>>>>> external tool" .
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I hear that someone need this RFE. So I want to discuss
>>>>>>>>>>>>>>>>>> about
>>>>>>>>>>>>>>>>>> this.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 2013/11/08 21:47, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Did someone read my mail?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I think that this RFE helps us to watch Java heap on
>>>>>>>>>>>>>>>>>>> production system.
>>>>>>>>>>>>>>>>>>> Also I think this RFE is able to be part of the JEP 158
>>>>>>>>>>>>>>>>>>> (Unified JVM Logging) .
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I want to update this RFE in JDK Bug System, but I
>>>>>>>>>>>>>>>>>>> don't
>>>>>>>>>>>>>>>>>>> have
>>>>>>>>>>>>>>>>>>> account.
>>>>>>>>>>>>>>>>>>> So I've posted email at first.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 2013/09/30 21:10, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>> In previous email, I've attached new patch for this
>>>>>>>>>>>>>>>>>>>> RFE.
>>>>>>>>>>>>>>>>>>>> It works fine with current hsx.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 2013/09/29 23:40, Yasu wrote:
>>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> We are using "logrotate" tool on RHEL for various log
>>>>>>>>>>>>>>>>>>>>> rotation.
>>>>>>>>>>>>>>>>>>>>> Current HotSpot has gclog rotation function for log
>>>>>>>>>>>>>>>>>>>>> size base,
>>>>>>>>>>>>>>>>>>>>> however I need to rotate gc log synchronizing with
>>>>>>>>>>>>>>>>>>>>> logrotate
>>>>>>>>>>>>>>>>>>>>> tool.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> So I've created RFE as "JDK-7090324: gclog
>>>>>>>>>>>>>>>>>>>>> rotation via
>>>>>>>>>>>>>>>>>>>>> external tool" .
>>>>>>>>>>>>>>>>>>>>> And Sr. Engineering Manager in Oracle said he use the
>>>>>>>>>>>>>>>>>>>>> essence of my patch in one
>>>>>>>>>>>>>>>>>>>>> of the jcmd subcommands.
>>>>>>>>>>>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2011-September/003274.html
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> 2 years ago, I posted a patch for this RFE.
>>>>>>>>>>>>>>>>>>>>> But this patch is too old to apply for current
>>>>>>>>>>>>>>>>>>>>> HotSpot.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> In last month, a similar discussion was appeared
>>>>>>>>>>>>>>>>>>>>> in ML.
>>>>>>>>>>>>>>>>>>>>> So I think it's time to discuss this RFE.
>>>>>>>>>>>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-August/008029.html
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Please cooperate.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Best regards,
>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic