RFR: 8262504: Some CLHSDB command cannot know they run on remote debugger

[Chris Plummer]

On Thu, 4 Mar 2021 07:14:18 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>>> This section seems to be the key to your changes, but I have to admit that I don't follow how it all ties in together. Maybe you can explain the overall structure of the Tool subclasses to help explain. They seem to be overloaded with modes that make it hard to follow all the logic. For example, you've added the `Tool(HotSpotAgent agent)` constructor, and we already have `Tool()` and `Tool(JVMDebugger d)`. It's unclear to me what the use case is for each. Also, we have about 10 subclasses of Tool. Why do only `PMap` an `PStack` need this new constructor.
>> 
>> All of traditional serviceability commands (jstack, jmap, etc...) seem to extend `Tool` because it has a function to parse commandline args and to attach debuggee commonly.
>> `Tool` supports several attach mechanism: `Tool()` supports attach debuggee manually, `Tool(JVMDebugger)` supports attach debuggee via available `JVMDegbugger`. `HotSpotAgent` communicates to debugee, so `Tool` would create its instance. Thus I added `Tool(HotSpotAgent)` to use available remote debug server.
>> 
>> AFAICS only `PMap` and `PStack` do not support remote debug server, so I changed for them.
>> (I plan to support `pmap` and `pstack` when we attach remote debug server, so it is important to know debuggee type.)
>> 
>>> BTW, looking at Tool and its subclasses has made me realize we have a whole other area of historical tool baggage, similar to what we are discussing with CLHSDB and HSDB. My guess is all these tool subclasses used to be invoked directly from the java command line. Now they are all hidden behind `jhsdb` subcommands, but still carry the old java command line support. In addition, when adding `jhsdb` and it's subcommands, it looks like there was an effort to mimic the existing Attach API commands like jmap and jinfo, so the SA versions of these commands have multiple modes which invoke different Tool subclasses. For example, `jhdsb jmap` alone can invoke the ClassLoaderStats, FinalizerInfo, ObjectHistogram, PMap, and HeapSummary tools, in addition to dumping the heap graph in HProf or GXL format. I think things would have been simpler and cleaner if each of these were made separate `jhsdb` subcommands, but it's probably too late for that. I suppose we could deprecate the existing su
 b-commands and add all these new sub-commands, but that will just make things harder for us until the old commands are eventually removed, which might never happen.
>> 
>> I agree with you it is too late to happen.
>> As I commented in https://github.com/openjdk/jdk/pull/2773#issuecomment-790203478 , we can do simplify for them as possible. For example, we can remove `main()` and related methods (e.g. showing usage) from them. If we can remove commandline support from all tool classes, It might help a little to maintain.
>
> If I understanding correctly, we have two users of Tools.java. One is `SALauncher` and the other is `CommandProcessor`. When we use `SALauncher`, we determine the `debugeeType` based on the command line arguments used. When using the `CommandProcessor`, we need access to the `HotSpotAgent` to get the `debuggeeType`, since it has stored this in its `startupMode` field . So the only reason for this new constructor is so we can set `debugeeType`, and then as you've pointed out, that makes the following existing code work correctly:
> 
>  if (getDebugeeType() == DEBUGEE_REMOTE) { 
>      out.println("remote configuration is not yet implemented"); 
>  } else { 
>      out.println("not yet implemented (debugger does not support CDebugger)!"); 
>  }

> I agree with you it is too late to happen.
> As I commented in [#2773 (comment)](https://github.com/openjdk/jdk/pull/2773#issuecomment-790203478) , we can do simplify for them as possible. For example, we can remove `main()` and related methods (e.g. showing usage) from them. If we can remove commandline support from all tool classes, It might help a little to maintain.

Yes, I'd like to see a lot of this excess baggage go away. I also wouldn't mind at least considering re-imagining the command line interface for all these Tool subclasses, but would only do so if we could quickly deprecate what we currently have. We already have enough cases of being able to do the same thing more than one way (I think in some cases 5 ways). We don't need one more, but on the other hand, as an example, having `jhsdb jmap` support dumping the java heap just doesn't make any sense. My guess is the first `jmap` just dumped the memory map of loaded libraries, and then more and more unrelated subcommands kept being added to it, but I'm not sure of the history. The Attach API `jmap` command suffers from the same issue, although oddly is does not support the library mapping output like the SA version does. I'm not sure which version of `jmap` came first and what it initially looked like.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2766