On Thu, 4 Mar 2021 02:08:58 GMT, Yasumasa Suenaga <ysuen...@openjdk.org> wrote:
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/Tool.java line 69: >> >>> 67: } >>> 68: } >>> 69: >> >> 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. >> >> 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. > >> 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. 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)!"); } ------------- PR: https://git.openjdk.java.net/jdk/pull/2766
