On Wed, 24 Feb 2021 04:12:13 GMT, Chris Plummer <[email protected]> wrote:
>>> It's too bad that the `dumpheap` command in previous JDKs has a slot for a >>> 3rd argument, but doesn't look at it. Maybe that was intentional to allow >>> for a new argument without causing a failure. What it means is if we want a >>> failure, we need to either send a different command, or use the suggestion >>> of having all the arguments in the first argument, both of which would >>> result in a failure when used with an older JDK (in which case we would >>> retry with older argument passing). >> >> Yes, I believe the argument max set to 3 was based on assumption that there >> can be at most 3 arguments from client side, no matter which tool it is >> (jmap, jstack, jinfo). >> >> And the jcmd command actually already passed all arguemnt as a single >> string, so there is no issue for accepting more arguments. >> >>> But they are far more convenient and easier to understand than jcmd, so >>> will probably stay around. But you are right in that they could still all >>> be converted to use jcmd internally. >> >> Maybe we could consider using `jcmd` internally to replace the current >> implementation of these tools, and keep their usage as before, then we get >> same result from `jcmd` and `jmap` alike tools, get rid of the argument >> passing limitation (because jcmd make all argument as a string), and also >> reduce the overhead of maintanence (e.g. don't bothering adding new options >> for both jmap and jcmd ) >> >>> Ok, but that fix is on the client side, not the target VM side. >> >> Yes, it only change the client side to reject illegal arguments, the JVM >> side still work as it is - ingore unknown argument silicently. However, the >> client side could at least reject unknown option and hence pass correct ones >> as expected to JVM. >> So if we agree on this change, at least for "parallel=" option, we don't >> need to retry when get the exception. :-) > >> > But they are far more convenient and easier to understand than jcmd, so >> > will probably stay around. But you are right in that they could still all >> > be converted to use jcmd internally. >> >> Maybe we could consider using `jcmd` internally to replace the current >> implementation of these tools, and keep their usage as before, then we get >> same result from `jcmd` and `jmap` alike tools, get rid of the argument >> passing limitation (because jcmd make all argument as a string), and also >> reduce the overhead of maintanence (e.g. don't bothering adding new options >> for both jmap and jcmd ) > > It does reduce the maintenance overhead, but you still need to parse the > arguments on the client side and package them up the way jcmd likes to see > them. > >> >> > Ok, but that fix is on the client side, not the target VM side. >> >> Yes, it only change the client side to reject illegal arguments, the JVM >> side still work as it is - ingore unknown argument silicently. However, the >> client side could at least reject unknown option and hence pass correct ones >> as expected to JVM. >> So if we agree on this change, at least for "parallel=" option, we don't >> need to retry when get the exception. :-) > > Actually you should still see an error when using `parallel` or `gz` when the > target VM doesn't understand them. This is because `jcmd` will reject them on > the target VM side. Then the question is whether to retry without them, or > pass the error to the user and let them adjust their `jmap` command. The > problem with the latter is that the arguments in the `jcmd` error message > might not look exactly the same as the way they were specified by the user to > `jmap`. We'd have take a closer look at the `jcmd` error messages to see if > they are appropriate for `jmap` users. Hi @plummercj, I agree, the output data must already be used by some parsing tools I believe. So the output format must keep the same no matter what change we want to make. I will mark CSR to this PR as we agreed the on introducing the `parallel=` option. Thanks ------------- PR: https://git.openjdk.java.net/jdk/pull/2261
