Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8275775: VM.metaspace prints flag 'f' for classes that have non-trivial finalize() #6075

Closed
wants to merge 5 commits into from

Conversation

kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Oct 22, 2021

Some customers want to observe which loaded classes have overridden the finalize() method. I found that VM.metaspace can output detailed classes. It seems feasible to add 'f' flag to it. With this patch, I found that ZipFileSystem left a finalize after applying this patch, which was obsolete 5 years ago, maybe we should remove it?


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8275775: VM.metaspace prints flag 'f' for classes that have non-trivial finalize()

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6075/head:pull/6075
$ git checkout pull/6075

Update a local copy of the PR:
$ git checkout pull/6075
$ git pull https://git.openjdk.java.net/jdk pull/6075/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6075

View PR using the GUI difftool:
$ git pr show -t 6075

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6075.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 22, 2021

👋 Welcome back yyang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Oct 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2021

@kelthuzadx The following labels will be automatically applied to this pull request:

  • hotspot-runtime
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added serviceability hotspot-runtime labels Oct 22, 2021
@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Oct 22, 2021

/label remove serviceability

@openjdk openjdk bot removed the serviceability label Oct 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2021

@kelthuzadx
The serviceability label was successfully removed.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 22, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Yi,

I think flagging that the class has a finalizer is potentially useful, but the way this is expressed in not quite right. has_finalizer() will be true if the class has a non-trivial finalize() method that will be invoked, either because the class defines it or the class inherits it. To simply refer to this as "overrides" (Note NOT overridded) is inaccurate as you could actually have a class that overrides finalize() with an empty implementation that replaces a non-trivial super-class finalize() method. While this is a questionable thing to do, such a class will not be marked as overriding finalize() even though it has in an important way.

I think the flag should be described as showing "has a non-trivial finalize() method".

Thanks,
David

@@ -42,11 +42,12 @@ void PrintMetaspaceInfoKlassClosure::do_klass(Klass* k) {
_out->print(UINTX_FORMAT_W(4) ": ", _cnt);

// Print a 's' for shared classes
_out->put(k->is_shared() ? 's': ' ');
_out->put(k->is_shared() ? 's' : ' ');
// Print 's' for classes that overrided finalize() method
Copy link
Member

@dholmes-ora dholmes-ora Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/'s'/'f'/

s/overrided/override/

Copy link
Member Author

@kelthuzadx kelthuzadx Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification. I skim through the code:

  if (!_has_empty_finalizer) {
    if (_has_finalizer ||
        (super != NULL && super->has_finalizer())) {
      ik->set_has_finalizer();
    }
  }

Yes, "has a non-trivial finalize() method" expresses more clear because a class is annotated with _has_finalize only if it has a non-trivial finalize method or its super has non-trivial finalize method.

_out->put(k->is_shared() ? 's': ' ');
_out->put(k->is_shared() ? 's' : ' ');
// Print 's' for classes that overrided finalize() method
_out->put(k->has_finalizer() ?'f' : ' ');
Copy link
Member

@dholmes-ora dholmes-ora Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: need space after ?


ResourceMark rm;
_out->print(" %s", k->external_name());

_out->print(" %s (" INTPTR_FORMAT ")", k->external_name(), p2i(k));
Copy link
Member

@dholmes-ora dholmes-ora Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the address printed?

Copy link
Member Author

@kelthuzadx kelthuzadx Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this would facilitate further debugging. I will remove them if you think they are not necessary.

Copy link
Member

@tstuefe tstuefe Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove it. Its not needed in this context.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 26, 2021

⚠️ @kelthuzadx This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Copy link
Member

@dholmes-ora dholmes-ora left a comment

A couple of spelling nits but otherwise ok. Thanks for the updates.

Please update the JBS title and PR title to refer to "non-trivial finalize()"

Thanks,
David

@@ -43,8 +43,8 @@ void PrintMetaspaceInfoKlassClosure::do_klass(Klass* k) {

// Print a 's' for shared classes
_out->put(k->is_shared() ? 's' : ' ');
// Print 's' for classes that overrided finalize() method
_out->put(k->has_finalizer() ?'f' : ' ');
// Print a 'f' for classes that having a non-trivial finalize() method
Copy link
Member

@dholmes-ora dholmes-ora Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/a/an/ (as 'f' is pronounced eff it gets treated as a vowel)
s/having/have/

Copy link
Member Author

@kelthuzadx kelthuzadx Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.. Good catch!

@openjdk
Copy link

@openjdk openjdk bot commented Oct 26, 2021

@kelthuzadx This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8275775: VM.metaspace prints flag 'f' for classes that have non-trivial finalize()

Reviewed-by: dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 174 new commits pushed to the master branch:

  • b0d1e4f: 8273585: String.charAt performance degrades due to JDK-8268698
  • 7addcd7: 8276034: ProblemList gtest dll_address_to_function_and_library_name on macosx-x64
  • 2448b3f: 8275874: [JVMCI] only support aligned reads in c2v_readFieldValue
  • f1f5e26: 8275872: Sync J2DBench run and analyze Makefile targets with build.xml
  • 19f76c2: 8275079: Remove unnecessary conversion to String in java.net.http
  • e5cd269: 8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info
  • 82f4aac: 8259609: C2: optimize long range checks in long counted loops
  • 574f890: 8275720: CommonComponentAccessibility.createWithParent isWrapped causes mem leak
  • 7c88a59: 8275809: crash in [CommonComponentAccessibility getCAccessible:withEnv:]
  • c9dec2f: 8273299: Unnecessary Vector usage in java.security.jgss
  • ... and 164 more: https://git.openjdk.java.net/jdk/compare/b8cb76ad210cb3e7524c7f5b13cfe57746ac05d4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 26, 2021
@kelthuzadx kelthuzadx changed the title 8275775: VM.metaspace prints flag 'f' for classes that overrided finalize() 8275775: VM.metaspace prints flag 'f' for classes that have non-trivial finalize() Oct 27, 2021
@openjdk openjdk bot added ready and removed ready labels Oct 27, 2021
Copy link
Member

@tstuefe tstuefe left a comment

Hi Yi,

I'm ambivalent because VM.metaspace is not a particularly good place for that info. VM.metaspace prints info related to the memory usage of metadata. s fits there because shared classes are not stored in Metaspace, which is a vital info when looking at the output (the numbers won't make sense otherwise). But the new f is just some arbitrary information about the class makeup with no particular influence on metaspace.

OTOH I don't know any other existing command better suited. We have GC.class_histogram, VM.class_hierarchy and VM.classloaders (also with show-classes option), all print out classes. But the same problem there, why add it there.

Are you sure VM.metaspace is what you want? I won't block this patch, if you are sure. But if we do this we should longterm find a better command for this info, or add a new command (VM.class_details or whatever).


A regression test is needed for this. Please adapt runtime/Metaspace/PrintMetaspaceDcmd.java:

  • add a test to test show-classes (we already test show-loaders so maybe you can piggyback on that one
  • test for a class you know has a non-trivial finalize to show up with f, and for one which has not to not show f
  • optionally, add @run configuration to run at least once with -Xshare:off to test how the output looks like when applied to a VM without CDS (so, no s should be printed, but your new f output should still show up)

Thank you,

Thomas


ResourceMark rm;
_out->print(" %s", k->external_name());

_out->print(" %s (" INTPTR_FORMAT ")", k->external_name(), p2i(k));
Copy link
Member

@tstuefe tstuefe Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove it. Its not needed in this context.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Oct 27, 2021

Hi Thomas and David,

Thanks a lot for your reviews.

I was convinced by above comments. VM.metaspace may not be a good place, because as explained that it prints info related to the memory usage of metadata. On the other hand, we do encounter some real scenarios to inspect InstanceKlass. jhsdb is not capable in product environment. I think jcmd VM.classes is a more suitable place to output this information as we already have VM.classloaders/VM.symboltable/VM.systemdictionary, it is also flexible for subsequent extension (e.g. print more flags and more detailed class information) and easy to use. In my preliminary idea, jcmd VM.classes outputs summary class information, just like what PrintCompilation did, jcmd VM.classes details outputs detailed class information. Any idea is appreciated.

Thanks!

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Oct 27, 2021

Hi Thomas and David,

Thanks a lot for your reviews.

I was convinced by above comments. VM.metaspace may not be a good place, because as explained that it prints info related to the memory usage of metadata. On the other hand, we do encounter some real scenarios to inspect InstanceKlass. jhsdb is not capable in product environment. I think jcmd VM.classes is a more suitable place to output this information as we already have VM.classloaders/VM.symboltable/VM.systemdictionary, it is also flexible for subsequent extension (e.g. print more flags and more detailed class information) and easy to use. In my preliminary idea, jcmd VM.classes outputs summary class information, just like what PrintCompilation did, jcmd VM.classes details outputs detailed class information. Any idea is appreciated.

Thanks!

I like this (also the succinct name). Unfortunately, this would need a CSR, but if you have not yet done that we can help with this.

Cheers, Thomas

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Oct 27, 2021

Hi Thomas and David,
Thanks a lot for your reviews.
I was convinced by above comments. VM.metaspace may not be a good place, because as explained that it prints info related to the memory usage of metadata. On the other hand, we do encounter some real scenarios to inspect InstanceKlass. jhsdb is not capable in product environment. I think jcmd VM.classes is a more suitable place to output this information as we already have VM.classloaders/VM.symboltable/VM.systemdictionary, it is also flexible for subsequent extension (e.g. print more flags and more detailed class information) and easy to use. In my preliminary idea, jcmd VM.classes outputs summary class information, just like what PrintCompilation did, jcmd VM.classes details outputs detailed class information. Any idea is appreciated.
Thanks!

I like this (also the succinct name). Unfortunately, this would need a CSR, but if you have not yet done that we can help with this.

Cheers, Thomas

That would help a lot! IIRC, I need to create an issue and associate a CSR. I could draft an initial version for that, I'm very glad that you can help revise it. But before that, let's see if David has more comments about this.

Thanks.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Oct 28, 2021

Do we have sufficient class information to combine with the finalize() status, remembering that we will be reporting on every single class, to make VM.classes worthwhile? Would a simple command to list all non-trivially-finalizable classes be useful in itself? VM.finalizable_classes?

1 similar comment
@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Nov 1, 2021

Do we have sufficient class information to combine with the finalize() status, remembering that we will be reporting on every single class, to make VM.classes worthwhile? Would a simple command to list all non-trivially-finalizable classes be useful in itself? VM.finalizable_classes?

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Nov 1, 2021

Hi David,

Do we have sufficient class information to combine with the finalize() status, remembering that we will be reporting on every single class, to make VM.classes worthwhile? Would a simple command to list all non-trivially-finalizable classes be useful in itself? VM.finalizable_classes?

VM.finalizable_classes seems to be too ad-hoc. The only purpose is to print the class that has non-trivial finalize() method. If, maybe one day in the future, we want to know which classes have miranda method, we may need to add another VM.miranda_classes command. VM.classes can print detailed information of all loaded classes of the JVM, which looks more extensible and flexible.

Thanks.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 2, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 1/11/2021 5:19 pm, Yi Yang wrote:

On Wed, 27 Oct 2021 01:30:37 GMT, Yi Yang <yyang at openjdk.org> wrote:

Some customers want to observe which loaded classes have overridden the finalize() method. I found that VM.metaspace can output detailed classes. It seems feasible to add 'f' flag to it. With this patch, I found that ZipFileSystem left a finalize after applying this patch, which was obsolete 5 years ago, maybe we should remove it?

Yi Yang has updated the pull request incrementally with one additional commit since the last revision:

typo

Hi David,

Do we have sufficient class information to combine with the finalize() status, remembering that we will be reporting on every single class, to make VM.classes worthwhile? Would a simple command to list all non-trivially-finalizable classes be useful in itself? VM.finalizable_classes?

VM.finalizable_classes seems to be too ad-hoc. The only purpose is to print the class that has non-trivial finalize() method. If, maybe one day in the future, we want to know which classes have miranda method, we may need to add another VM.miranda_classes command. VM.classes can print detailed information of all loaded classes of the JVM, which looks more extensible and flexible.

My concern is there will be too much information to be useful if you
simply list and flag every single loaded class. We are basically trying
to make a decision whether to add a command that asks a specific query
(show me all finalizable classes), versus a command that just produces a
class dump and the user then has to manually apply their query to that
raw data. There are pros and cons to each approach.

This also seems more of a serviceability issue so I've cc'd that mailing
list.

Also this now relates to the new JEP on finalization removal:

https://openjdk.java.net/jeps/421

as a tool to help users see where finalizers exist will be useful to
that effort.

Cheers,
David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 2, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 2/11/2021 7:55 am, David Holmes wrote:

On 1/11/2021 5:19 pm, Yi Yang wrote:

On Wed, 27 Oct 2021 01:30:37 GMT, Yi Yang <yyang at openjdk.org> wrote:

Some customers want to observe which loaded classes have overridden
the finalize() method. I found that VM.metaspace can output detailed
classes. It seems feasible to add 'f' flag to it. With this patch, I
found that ZipFileSystem left a finalize after applying this patch,
which was obsolete 5 years ago, maybe we should remove it?

Yi Yang has updated the pull request incrementally with one
additional commit since the last revision:

?? typo

Hi David,

Do we have sufficient class information to combine with the
finalize() status, remembering that we will be reporting on every
single class, to make VM.classes worthwhile? Would a simple command
to list all non-trivially-finalizable classes be useful in itself?
VM.finalizable_classes?

VM.finalizable_classes seems to be too ad-hoc. The only purpose is to
print the class that has non-trivial finalize() method. If, maybe one
day in the future, we want to know which classes have miranda method,
we may need to add another VM.miranda_classes command. VM.classes can
print detailed information of all loaded classes of the JVM, which
looks more extensible and flexible.

My concern is there will be too much information to be useful if you
simply list and flag every single loaded class. We are basically trying
to make a decision whether to add a command that asks a specific query
(show me all finalizable classes), versus a command that just produces a
class dump and the user then has to manually apply their query to that
raw data. There are pros and cons to each approach.

It has been pointed out (thanks Alex) that we already have
VM.class_hierarchy to print all classes, so perhaps that could be
augmented to add a decorator for finalizable classes?

David

This also seems more of a serviceability issue so I've cc'd that mailing
list.

Also this now relates to the new JEP on finalization removal:

https://openjdk.java.net/jeps/421

as a tool to help users see where finalizers exist will be useful to
that effort.

Cheers,
David

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Nov 2, 2021

I like this (also the succinct name). Unfortunately, this would need a CSR, but if you have not yet done that we can help with this.

@tstuefe We do not use CSR requests with jcmd changes as they are deemed diagnostic commands - ref JDK-8203682 :)

@iklam
Copy link
Member

@iklam iklam commented Nov 2, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 2/11/2021 7:55 am, David Holmes wrote:

On 1/11/2021 5:19 pm, Yi Yang wrote:

On Wed, 27 Oct 2021 01:30:37 GMT, Yi Yang wrote:

Some customers want to observe which loaded classes have overridden
the finalize() method. I found that VM.metaspace can output detailed
classes. It seems feasible to add 'f' flag to it. With this patch, I
found that ZipFileSystem left a finalize after applying this patch,
which was obsolete 5 years ago, maybe we should remove it?

Yi Yang has updated the pull request incrementally with one
additional commit since the last revision:
?? typo

Hi David,

Do we have sufficient class information to combine with the
finalize() status, remembering that we will be reporting on every
single class, to make VM.classes worthwhile? Would a simple command
to list all non-trivially-finalizable classes be useful in itself?
VM.finalizable_classes?

VM.finalizable_classes seems to be too ad-hoc. The only purpose is to
print the class that has non-trivial finalize() method. If, maybe one
day in the future, we want to know which classes have miranda method,
we may need to add another VM.miranda_classes command. VM.classes can
print detailed information of all loaded classes of the JVM, which
looks more extensible and flexible.

My concern is there will be too much information to be useful if you
simply list and flag every single loaded class. We are basically trying
to make a decision whether to add a command that asks a specific query
(show me all finalizable classes), versus a command that just produces a
class dump and the user then has to manually apply their query to that
raw data. There are pros and cons to each approach.

It has been pointed out (thanks Alex) that we already have VM.class_hierarchy to print all classes, so perhaps that could be augmented to add a decorator for finalizable classes?

David

This also seems more of a serviceability issue so I've cc'd that mailing
list.
Also this now relates to the new JEP on finalization removal:
https://openjdk.java.net/jeps/421
as a tool to help users see where finalizers exist will be useful to
that effort.
Cheers,
David

In general, I think it’s better to have a generic command that can be easily adapted by the user to look for the desired information. We can augment an existing command that already prints out all classes to add the finalization info. It should be pretty easy to grep for “finalizable”, etc, from the output. I would not worry about “too much information being printed”. That can be dealt pretty easily.

For usability, it would be much better than to ask the user to remember many distinct commands for querying related but different information. Also, the VM implementation will be simpler.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Nov 2, 2021

Personally and ideally, I hope JCMD can provide several types of general commands, allow users to quickly find what they want, and then we can add more subcommands for advanced usages.

For example, we provide:

VM.classes
VM.classloaders
VM.stringtable
VM.symboltable
VM.cds
VM.metaspace

It’s easy to know what it does and what users want. Then we add some subcommands for debugging/diagnostic support. In this way, we can further simplify the current commands, such as:

  1. VM.classloaders = VM.classloaders + VM.classloader_stats

VM.classloaders prints current class loaders(VM.classloader_stats), we can also provide some advanced argument such as "hierarchy"(actually what VM.classloaders does now), "details" to print more information.

  1. VM.command_line += VM.flags

VM Arguments:
jvm_args: -agentlib:jdwp=transport=dt_socket,address=localhost:23158
jvm_flag: <<<<>>>
java_command: jdk.jshell.execution.RemoteExecutionControl 33170
java_class_path (initial):.
Launcher Type: SUN_STANDARD

Roughly thoughts.

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Nov 2, 2021

I like this (also the succinct name). Unfortunately, this would need a CSR, but if you have not yet done that we can help with this.

@tstuefe We do not use CSR requests with jcmd changes as they are deemed diagnostic commands - ref JDK-8203682 :)

Seems odd since these are outside interfaces, used in scripts and whatnot. IMHO one of the places where CSR really makes sense.

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Nov 2, 2021

Personally and ideally, I hope JCMD can provide several types of general commands, allow users to quickly find what they want, and then we can add more subcommands for advanced usages.

For example, we provide:

VM.classes
VM.classloaders
VM.stringtable
VM.symboltable
VM.cds
VM.metaspace

It’s easy to know what it does and what users want. Then we add some subcommands for debugging/diagnostic support. In this way, we can further simplify the current commands, such as:

  1. VM.classloaders = VM.classloaders + VM.classloader_stats

VM.classloaders prints current class loaders(VM.classloader_stats), we can also provide some advanced argument such as "hierarchy"(actually what VM.classloaders does now), "details" to print more information.

Both commands do different things. VM.classloaders prints out a tree-like structure, not a table. I added VM.classloaders
because we needed a quick way at SAP to display our sometimes insanely complex loader hierarchy. Printing that tree-like structure requires a different logic for printing a simple table, since it is not stateless (see classLoaderHierarchyDCmd.cpp).
Code-wise I doubt you save a lot complexity by combining those commands. As for simplifying user-facing commands, it is a matter of taste, see below (but note that changing existing commands introduces backward compatibility issues).

  1. VM.command_line += VM.flags

VM Arguments: jvm_args: -agentlib:jdwp=transport=dt_socket,address=localhost:23158 jvm_flag: <<<<>>> java_command: jdk.jshell.execution.RemoteExecutionControl 33170 java_class_path (initial):. Launcher Type: SUN_STANDARD

Roughly thoughts.

Do you want tiny minimalist commands doing one thing and that well? Or do you want a meta-command for one area and hide all the functionality in sub options? It is a matter of taste, mostly.

There are some advantages to the minimalist approach. E.g. you can just query the hotspot for capabilities by getting a list of commands. That makes outside scripting wrt different hotspot versions easier. But with a meta-command, you don't have that.

We have similar decisions in other areas too, e.g. command line switches. Most hotspot command line switches are atomar. They describe one thing. But there are meta-switches, e.g. -Xlog or -XX:StartFlightRecording. There, a bunch of sub options was packed under one flag, requiring extra parsing code for these and, for me, are always more difficult since they follow an own logic (e.g. I always have to look up Xlog syntax).

I'm not saying jcmd is perfect. To me, jcmd feels like a framework where we never really agreed on one syntax, and as a result the commands feel a bit arbitrary. E.g. we have meta commands (VM.metaspace) and atomar commands. But instead of combining commands, maybe one could introduce command groups. E.g
VM.classloaders.stats. VM.classloaders.tree. Etc.

Even only a visual separation when printing the command list would help already. E.g. combining all classloader-related commands under one header. Though deciding what goes where may be difficult, e.g. for VM.metaspace.

Just a thought.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Nov 2, 2021

Personally and ideally, I hope JCMD can provide several types of general commands, allow users to quickly find what they want, and then we can add more subcommands for advanced usages.
For example, we provide:

VM.classes
VM.classloaders
VM.stringtable
VM.symboltable
VM.cds
VM.metaspace

It’s easy to know what it does and what users want. Then we add some subcommands for debugging/diagnostic support. In this way, we can further simplify the current commands, such as:

  1. VM.classloaders = VM.classloaders + VM.classloader_stats

VM.classloaders prints current class loaders(VM.classloader_stats), we can also provide some advanced argument such as "hierarchy"(actually what VM.classloaders does now), "details" to print more information.

Both commands do different things. VM.classloaders prints out a tree-like structure, not a table. I added VM.classloaders because we needed a quick way at SAP to display our sometimes insanely complex loader hierarchy. Printing that tree-like structure requires a different logic for printing a simple table, since it is not stateless (see classLoaderHierarchyDCmd.cpp). Code-wise I doubt you save a lot complexity by combining those commands. As for simplifying user-facing commands, it is a matter of taste, see below (but note that changing existing commands introduces backward compatibility issues).

  1. VM.command_line += VM.flags

VM Arguments: jvm_args: -agentlib:jdwp=transport=dt_socket,address=localhost:23158 jvm_flag: <<<<>>> java_command: jdk.jshell.execution.RemoteExecutionControl 33170 java_class_path (initial):. Launcher Type: SUN_STANDARD
Roughly thoughts.

Do you want tiny minimalist commands doing one thing and that well? Or do you want a meta-command for one area and hide all the functionality in sub options? It is a matter of taste, mostly.

There are some advantages to the minimalist approach. E.g. you can just query the hotspot for capabilities by getting a list of commands. That makes outside scripting wrt different hotspot versions easier. But with a meta-command, you don't have that.

We have similar decisions in other areas too, e.g. command line switches. Most hotspot command line switches are atomar. They describe one thing. But there are meta-switches, e.g. -Xlog or -XX:StartFlightRecording. There, a bunch of sub options was packed under one flag, requiring extra parsing code for these and, for me, are always more difficult since they follow an own logic (e.g. I always have to look up Xlog syntax).

I'm not saying jcmd is perfect. To me, jcmd feels like a framework where we never really agreed on one syntax, and as a result the commands feel a bit arbitrary. E.g. we have meta commands (VM.metaspace) and atomar commands. But instead of combining commands, maybe one could introduce command groups. E.g VM.classloaders.stats. VM.classloaders.tree. Etc.

Even only a visual separation when printing the command list would help already. E.g. combining all classloader-related commands under one header. Though deciding what goes where may be difficult, e.g. for VM.metaspace.

Just a thought.

I agree that this is a matter of taste. I prefer the minimalist command list, but I don't have a strong desire to make such changes, just a rough idea. The current jcmd commands are somewhat arbitrary, we are able to normalize it in some way in the future.

Going back to our topic, I think if possible, using the new command VM.classes instead of adding decorators to VM.class_hierarchy might be a better choice.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 2, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 2/11/2021 3:26 pm, Thomas Stuefe wrote:

On Tue, 2 Nov 2021 00:14:48 GMT, David Holmes <dholmes at openjdk.org> wrote:

I like this (also the succinct name). Unfortunately, this would need a CSR, but if you have not yet done that we can help with this.

@tstuefe We do not use CSR requests with jcmd changes as they are deemed diagnostic commands - ref JDK-8203682 :)

Seems odd since these are outside interfaces, used in scripts and whatnot. IMHO one of the places where CSR really makes sense.

From [1]

David > 1. Have we previously established whether a CSR request is
needed for a new Dcmd?

Thomas > "My feeling is no, since this adds a new command, so there can
be no backward compat issues."

:)

[1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-May/028190.html

Everyone is entitled to change their mind of course, but new commands
don't have compatibility issues as you point out. And diagnostic
interfaces don't need to go through the CSR process. So ...

Cheers,
David

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Nov 2, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 2/11/2021 3:26 pm, Thomas Stuefe wrote:

On Tue, 2 Nov 2021 00:14:48 GMT, David Holmes wrote:

I like this (also the succinct name). Unfortunately, this would need a CSR, but if you have not yet done that we can help with this.

@tstuefe We do not use CSR requests with jcmd changes as they are deemed diagnostic commands - ref JDK-8203682 :)

Seems odd since these are outside interfaces, used in scripts and whatnot. IMHO one of the places where CSR really makes sense.

From [1]

David > 1. Have we previously established whether a CSR request is needed for a new Dcmd?

Thomas > "My feeling is no, since this adds a new command, so there can be no backward compat issues."

:)

[1] http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-May/028190.html

Everyone is entitled to change their mind of course, but new commands don't have compatibility issues as you point out. And diagnostic interfaces don't need to go through the CSR process. So ...

Cheers, David

So let me try to do that~ People can still leave their comments on new PR. Thanks.

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Nov 2, 2021

@tstuefe We do not use CSR requests with jcmd changes as they are deemed diagnostic commands - ref JDK-8203682 :)

Seems odd since these are outside interfaces, used in scripts and whatnot. IMHO one of the places where CSR really makes sense.

From [1]

David > 1. Have we previously established whether a CSR request is needed for a new Dcmd?

Thomas > "My feeling is no, since this adds a new command, so there can be no backward compat issues."

:)

Hah, that's embarrassing :-)

I actually changed my mind with the asynchronous -Xlog discussions recently (https://bugs.openjdk.java.net/browse/JDK-8229517). CSR seemed to work well here, a good way to get more eyes for the user-facing command syntax. But I know that CSR is not the perfect process either (slow, arduous, and arguably new commands are not a backward compatibility issue).

@iklam
Copy link
Member

@iklam iklam commented Nov 2, 2021

Thomas > "My feeling is no, since this adds a new command, so there can be no backward compat issues."
:)

Hah, that's embarrassing :-)

I actually changed my mind with the asynchronous -Xlog discussions recently (https://bugs.openjdk.java.net/browse/JDK-8229517). CSR seemed to work well here, a good way to get more eyes for the user-facing command syntax. But I know that CSR is not the perfect process either (slow, arduous, and arguably new commands are not a backward compatibility issue).

I don't know about jcmd, but in general new interfaces should be discussed in CSR. We don't want to get stuck with an interface that's difficult to evolve (e.g., have forward compatibility issues).

https://wiki.openjdk.java.net/display/csr/CSR+FAQs

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 2, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 2/11/2021 5:23 pm, Ioi Lam wrote:

On Tue, 2 Nov 2021 06:45:55 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

Thomas > "My feeling is no, since this adds a new command, so there can be no backward compat issues."
:)

Hah, that's embarrassing :-)

I actually changed my mind with the asynchronous -Xlog discussions recently (https://bugs.openjdk.java.net/browse/JDK-8229517). CSR seemed to work well here, a good way to get more eyes for the user-facing command syntax. But I know that CSR is not the perfect process either (slow, arduous, and arguably new commands are not a backward compatibility issue).

I don't know about jcmd, but in general new interfaces should be discussed in CSR. We don't want to get stuck with an interface that's difficult to evolve (e.g., have forward compatibility issues).

https://wiki.openjdk.java.net/display/csr/CSR+FAQs

jcmd has been considered a diagnostic interface and therefore outside of
the CSR purview. But that's not to say that we don't need to think
carefully about any changes, whether new commands, or changes to
existing ones.

David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime ready rfr
4 participants