On Wed, 3 Mar 2021 23:42:31 GMT, Chris Plummer <cjplum...@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 sub- 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/2766
