Hi Frederic,

On Thu, May 24, 2018 at 7:29 PM, Frederic Parain
<frederic.par...@oracle.com> wrote:
> Hi Thomas,
>
>
> On 05/24/2018 12:08 PM, Thomas Stüfe wrote:
>>
>> Hi Erik,
>>
>> On Tue, May 22, 2018 at 11:08 PM, Erik Gahlin <erik.gah...@oracle.com>
>> wrote:
>>>
>>> Not sure if this will have an impact, but we have plans to add
>>> parameterization to JFR.start.
>>>
>>> It will work like the recording wizard in JMC. All the the widgets you
>>> see
>>> in the dialog will be available from command line. For instance, you will
>>> be
>>> able to do something like this (syntax and naming have not been decided)
>>>
>>> jcmd <pid> JFR.start gc-level=detailed exceptions=true
>>> method-sampling=10ms
>>> classloading=true
>>>
>>> The parameters are not fixed at compile time, but instead depends on the
>>> .jfc file in use. This will simplify usage of JFR as you will no longer
>>> need
>>> to export a .jfc file from JMC if you want to configure events. The
>>> parameterization will also be available from -XX:StartFlightRecording,
>>> which
>>> uses a comma-separated list.
>>>
>>> Furthermore, JFR also supports custom settings, which can have arbitrary
>>> syntax and semantics. For instance, it will be possible to add a setting
>>> that will filter out events for a particular thread and have it exposed
>>> to
>>> jcmd as a parameter, i.e.
>>>
>>> jcmd <pid> JFR.start threads={Thread1,Thread2, .*Pool.*}
>>>
>>> The diagnostic commands for JFR are implemented in Java, so they will not
>>> be
>>> able to execute during a safepoint, so I don't expect there to be an
>>> issue,
>>> but I thought I should mention it.
>>
>>
>> Yes, it is good to know that.
>>
>> There are two different problems here:
>> 1) how to specify multiple jcmd commands in a convenient way on the
>> command line
>> 2) how to run them in a single safepoint.
>>
>>
>> For (1) it turns out it already works, in a fashion:
>>
>> We can specify multiple commands when they are separated by newlines.
>> I think that is an unintended side effect of the command file support
>> (-f), but surprisingly also works on the command line if you surround
>> all arguments with a single quote:
>>
>> thomas@mainframe /shared/projects/openjdk/jdk-jdk/output-fastdebug $
>> ./images/jdk/bin/jcmd test3 'VM.version
>> VM.metaspace basic
>> VM.uptime'
>
>
> As the author of this code, I confirm that it is an unintended side effect.
>
>> which is of course completely unix-unlike and inconvenient to use. It
>> just works because jcmd runner appends all program arguments to a
>> single string, which it sends over to the VM.
>>
>> --
>>
>> My proposal would be that we change that in a backward-compatible way
>> by specifying that
>> - multiple commands are allowed without having to be separated by
>> newlines. Any whitespace should be a separator:
>> <command>[ <parameter> <parameter>] <command>[ <parameter> <parameter>]
>> - a command name indicates start of a new command. Any token which is
>> not a command name is a parameter for the preceeding command.
>
>
> This syntax, without special separator, would be hard to implement.
> The command line string is split into individual commands without
> any knowledge about the particular syntax of each command (see
> DCmd::parse_and_execute()). Considering all command name as a
> separator would significantly complexify the initial parsing.

No, its not that hard, one just has to modify the parsing a bit.

But talk is cheap :)

so here is a working prototype:

http://cr.openjdk.java.net/~stuefe/webrevs/jcmd-stack-commands/webrev.00/webrev/

which modifies parsing in such a way that instead of newlines the
commands are split at the start of the next command. Using that, I can
specify multiple commands, including commands with parameters, at the
command line, and they will processed sequentially:

./images/jdk/bin/jcmd test3 VM.uptime VM.metaspace basic
VM.classloaders verbose VM.uptime
19857:
25,108 s

Usage:
  Non-class:      4,47 MB capacity,     4,43 MB (>99%) used,    41,36
KB ( <1%) free+waste,     1,56 KB ( <1%) overhead.
      Class:    392,00 KB capacity,   377,27 KB ( 96%) used,    14,23
KB (  4%) free+waste,   512 bytes ( <1%) overhead.
       Both:      4,86 MB capacity,     4,80 MB ( 99%) used,    55,59
KB (  1%) free+waste,     2,06 KB ( <1%) overhead.

Virtual space:
  Non-class space:        8,00 MB reserved,       4,75 MB ( 59%) committed
      Class space:        1,00 GB reserved,     512,00 KB ( <1%) committed
             Both:        1,01 GB reserved,       5,25 MB ( <1%) committed

Chunk freelists:
   Non-Class:  4,25 KB
       Class:  0 bytes
        Both:  4,25 KB

+-- <bootstrap>
      |
      |                       CLD: 0x00007f75a41d85d0
      |              Loader Klass: 0x0000000000000000
      |                Loader Oop: 0x0000000000000000
      |
      +-- platform (instance of
jdk.internal.loader.ClassLoaders$PlatformClassLoader)
            |
            |                       CLD: 0x00007f75a439b9c0
            |              Loader Klass: 0x00000008000107b0
            |                Loader Oop: 0x0000000715d4cb20
            |
            +-- app (instance of
jdk.internal.loader.ClassLoaders$AppClassLoader)

                                          CLD: 0x00007f75a438f8e0
                                 Loader Klass: 0x0000000800010348
                                   Loader Oop: 0x0000000715d4efe0

25,108 s

(Note: VM.classloaders command is not yet upstream, I was searching
for a command with parameters and short output)

I am not saying that is what we should do, I am just saying it is not
difficult to implement.

Note that the patch does not touch the Safepoint issue, apart from
introducing DCmd::can_run_at_safepoint() which could be implemented by
each command similar to the other virtual functions and could be used
in parse_and_execute to decide command bundling.

>
>> This should work for JFR too, if your parameters do not contain
>> whitespace-enclosed strings which equal valid command names:
>>
>> JFR.start threads={Thread1,Thread2, .*Pool.*, VM.info}
>>
>> would be invalid since "VM.info" is a valid command name.
>>
>> If that proposal does not work, an explicit notation may be needed,
>> e.g. the initially mentioned enclosing-by-brackets:
>>
>> jcmd { VM.version, VM.metaspace basic, VM.uptime}
>>
>> ----
>>
>> 2) Yes, there are commands which cannot be run at a safepoint. I would
>> make that a property of the DiagnosticCmd object, and just forbid to
>> stack them with other commands.
>
>
> This is another big issue. The DCmd framework imposes very little
> limitation to what command can go. Any command invoking Java code,
> like ManagementAgent.start, would have troubles if executed at a
> safepoint.

As sketched out, I would envison a DCmd::can_run_at_safepoint()
virtual function, where each command can decide if it 1) needs a
safepoint, 2) does not care or 3) cannot run in a safepoint. Actually,
that way we could even centralize safepoint handling out of the
individual commands into the central loop, at least for all commands
which need safepoints.

>
>> This is all still very much in the thinking phase. The more I think
>> about this the clearer it gets that implementing it is the smallest
>> part - more time consuming will be to file a CSR and agree on a syntax
>> which is both backward compatible and practical on the unix command
>> line.
>
>
> Overall, I like the idea of grouping commands inside a single safepoint
> to provide consistency across their results. However, I think this
> behavior should be explicitly requested on the command line.

Yes, I get the feeling too that it may be better to let the user
explicitly bundle commands for one-safepoint-execution. Would that
make jcmd too difficult to use? Who knows about safepoints?

One argument in favor of letting the user decide is that running
multiple lengthy commands at a safepoint may trigger the safepoint
timeout. So, this could be a reason why one would want to avoid
automatic-command-bundling. I don't know... that is bikeshed
territory.

> Today, commands are not necessarily executed at a safepoint. Each
> command has to request a safepoint if it needs it.
>
> A way this could work would be to have:
>   - a special command to specify that all following commands in the
>     command string must be executed within the same safepoint
>   - this special safepoint will check that the following commands
>     are safe to be executed at a safepoint (this must be a new
>     property of each command as state by Thomas). If any command
>     is not safepoint-safe, the whole command string would be
>     rejected
>   - then this special command would trigger a safepoint, and then
>     invoke the following commands sequentially

I think whether to implement this as another command or to bake this
into the parser some other way is an implementation detail. I would
rather start out defining the syntax, which has to be backward
compatible (so, the response of a new VM with the new stacking
capability to an old jcmd talking to it must remain unchanged). This
needs a CSR and probably a long discussion. If my past experiences are
a guide, this will take time.

>
> A new separator, more convenient than newline, would be require to have
> a single line syntax.
>
> My 2 cents,
>

Certainly worth more than 2 cents :) Thanks for your thoughts!

..Thomas

> Fred
>
>
>>
>>>
>>> Erik
>>>
>>> Hi Thomas,
>>>
>>> very positive suggestion.
>>>
>>> One observation:
>>> There already seems to be some different interpretation of the command
>>> line
>>> in different Java versions:
>>> For instance when I try to dump a flight recording in 1.8.0_152-ea-b05 :
>>> I
>>> can use:
>>> jcmd 33014 JFR.dump filename="recording1.jfr" recording=1
>>> but in build 9+181 , the same command must be:
>>> jcmd 33014 JFR.dump filename="recording1.jfr",recording=1
>>> (the comma to separate sub-options you mentioned earlier)
>>>
>>> Therefore the suggestion without curly brackets, giving several commands
>>> after one another seems good with regard to backwards compatibility.
>>>
>>> Motivation: hawt.io uses the MBean
>>> com.sun.management:type=DiagnosticCommand
>>> to access the same functionality as jcmd. Variations in option syntax
>>> across
>>> Java versions is already an issue (only affecting sub-options though, as
>>> each command is a separate JMX operation). So syntax compatibility is
>>> highly
>>> appreciated :-)
>>>
>>> Martin
>>>
>>> lør. 12. mai 2018 kl. 20:11 skrev Kirk Pepperdine
>>> <kirk.pepperd...@gmail.com>:
>>>>
>>>>
>>>>
>>>>> On May 10, 2018, at 11:26 AM, Thomas Stüfe <thomas.stu...@gmail.com>
>>>>> wrote:
>>>>>
>>>>> On Thu, May 10, 2018 at 9:13 AM, Kirk Pepperdine
>>>>> <kirk.pepperd...@gmail.com> wrote:
>>>>>>
>>>>>> The stacking at the safe point would be a huge win. Right now thread
>>>>>> dump consistency can really confuse people as the tooling will show
>>>>>> two
>>>>>> threads owning the same lock at seemingly the same time. Reality, it’s
>>>>>> just
>>>>>> that people didn’t realize you get a safe point for each thread
>>>>>> therefor
>>>>>> there is motion in the system as you’re collecting the data.
>>>>>
>>>>>
>>>>> I am a bit confused. What tooling are you talking about?
>>>>
>>>>
>>>> jstack. I’ve not seen it with jcmd. But I often see 2 threads holding
>>>> the
>>>> same lock at the “same time” which is of course non-sense. I can dig one
>>>> up
>>>> for you if you’re still confused.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> As an aside, it’s amazing how many dev’s aren’t aware of jcmd. Just
>>>>>> yesterday after my session at Devoxx I had someone ask me about using
>>>>>> jfr
>>>>>> from the command line, many in that session had not seen jcmd before.
>>>>>> The
>>>>>> feedback was, wow, this is very useful and I wished I had of known
>>>>>> about it
>>>>>> earlier.
>>>>>
>>>>>
>>>>> Yes, jcmd is quite useful. I also really like the simple design, which
>>>>> is by default up- and downward compatible (so you can talk to any VM,
>>>>> new, old, it should not matter). That is just good design. We - Sap -
>>>>> work to extend jcmd, to help our support folks. E.g. the whole new
>>>>> VM.metaspace command chain.
>>>>
>>>>
>>>> And simple it is….well done!!!
>>>>
>>>> — Kirk
>>>>
>>>
>

Reply via email to