[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S): 8186902: jcmd GC.run should not be blocked by DisableExplicitGC
From:       Mikael Gerdin <mikael.gerdin () oracle ! com>
Date:       2017-08-30 7:10:03
Message-ID: a2f58fc4-b62f-3c4a-95d7-3cd3aaff492d () oracle ! com
[Download RAW message or body]

Hi Kevin,

On 2017-08-29 13:41, Kevin Walls wrote:
> Hi,
> 
> This is a small review request for:
> 
> 8186902: jcmd GC.run should not be blocked by DisableExplicitGC
> https://bugs.openjdk.java.net/browse/JDK-8186902
> 
> jcmd GC.run to invoke GC fails if -XX:+DisableExplicitGC is set: this 
> seems like a mistake as it is obstructive for a live app that needs a 
> GC, and was started with -XX:+DisableExplicitGC.
> 
> hg diff pasted below, simply removes the DisableExplicitGC if and 
> un-indents the existing call to collect().
> 
> Builds and manually tests OK, GC occurs in response to jcmd GC.run even 
> if java was started with -XX:+DisableExplicitGC
> 
> Previous email on this: 
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-August/021748.html 
> 
> 
> Many thanks
> Kevin
> 
> 
> bash-4.2$ cd hotspot
> bash-4.2$ hg diff
> diff -r a20f0fa4c426 src/share/vm/services/diagnosticCommand.cpp
> --- a/src/share/vm/services/diagnosticCommand.cpp       Mon Aug 28 
> 23:46:22 2017 +0000
> +++ b/src/share/vm/services/diagnosticCommand.cpp       Tue Aug 29 
> 02:55:56 2017 -0700
> @@ -414,11 +414,7 @@
>   }
> 
>   void SystemGCDCmd::execute(DCmdSource source, TRAPS) {
> -  if (!DisableExplicitGC) {
> -    Universe::heap()->collect(GCCause::_dcmd_gc_run);
> -  } else {
> -    output()->print_cr("Explicit GC is disabled, no GC has been 
> performed.");
> -  }
> +  Universe::heap()->collect(GCCause::_dcmd_gc_run);
>   }
> 
>   void RunFinalizationDCmd::execute(DCmdSource source, TRAPS) {
> bash-4.2$
> 
> 

Looks fine.
/Mikael

> 
> 
> On 29/08/2017 10:14, Kevin Walls wrote:
>>
>> Hi Mikael, thanks yes, it could be a separate cmd GC.runForce... 
>> However I was thinking if you can get as far as having your jcmd 
>> executed, you really _do_ want to run that collection.  Whatever 
>> behaviour you were protecting against when you chose the command-line 
>> arguments, you would only ever want to override if you run the jcmd to 
>> invoke GC... 8-)
>>
>> I'll convert this to a review request for removing that check, will 
>> post that shortly.  This would be changing the behaviour, but I don't 
>> think it contradicts anything we document, and we seem to have added 
>> the check without documenting it.
>>
>> Thanks
>> Kevin
>>
>>
>> On 28/08/2017 16:01, Mikael Gerdin wrote:
>>> Hi Kevin,
>>>
>>> On 2017-08-22 16:38, Kevin Walls wrote:
>>>> Hi,
>>>>
>>>> jcmd GC.run to invoke GC fails if -XX:+DisableExplicitGC is set: 
>>>> this seems like a mistake?
>>>>
>>>> This behaviour is obstructive for a live app that _needs_ a GC, and 
>>>> was started with -XX:+DisableExplicitGC.
>>>>
>>>> DisableExplicitGC to protect from Java code calling System.gc 
>>>> frequently makes sense, but if I can attach and run a dcmd, I should 
>>>> have permission to inspect and maintain the JVM, including invoking 
>>>> a GC. (This is as the user who owns the process and can kill it off.)
>>>>
>>>> The behaviour (checking DisableExplicitGC in SystemGCDCmd::execute) 
>>>> comes in with:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8004095
>>>> 8004095: Add support for JMX interface to Diagnostic Framework and 
>>>> Commands
>>>>
>>>> The JMX relation I suppose suggests we didn't want JMX to override 
>>>> DisableExplicitGC by way of using a jcmd/DCmd.
>>>>
>>>> But also, we now have:
>>>> 8072913: [REDO] GCCause should distinguish jcmd GC.run from System.gc()
>>>> Summary: GCCause which is caused by GC.run diagnostic command should 
>>>> be different from System.gc() .
>>>>
>>>> ..at least the causes are distinct.
>>>>
>>>> I don't think we document this clearly.  Our comment in globals.hpp 
>>>> is ""Ignore calls to System.gc()".  I don't think we say anywhere 
>>>> that jcmd is subject to being disabled by the flag.
>>>>
>>>> Interested to hear any reason in favour of the current behaviour! If 
>>>> there's nothing, I'll log a bug and ask for review of the change to 
>>>> remove it...
>>>
>>> There were some discussions earlier around this area and I came up 
>>> with the idea of having a "force" option to the GC.run command to 
>>> override DisableExplicitGC.
>>> The comments in globals.hpp are a notoriously bad spec for the flags 
>>> since they are only ever present in debug builds of the JVM.
>>>
>>> Thanks
>>> /Mikael
>>>
>>>>
>>>> Thanks
>>>> Kevin
>>>>
>>>>
>>>>
>>
> 
[prev in list] [next in list] [prev in thread] [next in thread]