-
Notifications
You must be signed in to change notification settings - Fork 57
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
[FEATURE] Explore changing logic to capture thread dump in PA #161
Comments
To be picked up by @@kaushalmahi12 once onboarded |
Did some research around this and here's what I have to add : The description of the issue is misleading, since it points the problem being the typecasting, but infact the problem is the class HotSpotVirtualMachine being inaccessible In the above two images you can see the module properties expose only the package we are solving this issue using --add-opens which is used to open a package and make all its types and members accessible - more on this The correct problem statementCan we avoid using the inaccessible class HotSpotVirtualMachine and hence avoid --addopens ? |
@kiranprakash154 |
Hey @kkhatua, @kiranprakash154 Edit: Just realized that it doesn't contain thread native id :/ . |
I explored a bit and found out the following: Apparently this package and interface were hidden on purpose because they're prone to change and not recommended to be used by external packages. Most of the command tools providing JVM profiling and dumping rely on it as well, i.e. jcmd, jstat : Example jcmd implementation. On internet, there are not much use cases and examples of mapping JVM-assigned thread id to native thread id's and not even of utilizing native thread id, especially programmatically (usually it is done through command line tools like jcmd). So far, I haven't been able to find a "non-hacky" way to reformat the existing logic and keep the native id. One of possible hacks could be Runtime execution of a separate jcmd process and consuming it's output stream (that would by the way result in the same output we're currently getting through HotSpotVirtualMachine). From one side this may not be a bad idea as jcmd is shipped with jdk and will follow the internal implementations and can provide us with relatively stable interface to these functionalities. From the other this is not clean and there are probably consequences of doing things this way inside OpenSearch environment and similar that I'm not aware of so please give your thoughts about it (probably also performance ?). Second one is even more convoluted and would imply using JNI or JNA to dynamically load code that does a proxy system call and gets native id of a thread, something similar to this, and is of course extremely system specific so proxy calls would vary for every operating system. This dl would have to be dragged around and it would also be hell for deployment, making this option bad imo. Third option is keeping the current --addopens hack and the fourth one would be dropping this functionality and transitioning to MXBean completely which we're already using for other dumping info. |
Hi @Tjofil, Thanks for your detailed findings.
ProcessBuilder lets you run commands and produce output into a file. performance-analyzer-rca/src/main/java/org/opensearch/performanceanalyzer/jvm/ThreadList.java Lines 324 to 361 in c020cc9
After attaching to the vm, we have to load a jar which does all of these things.
It would look something like this. |
Hey @kiranprakash154,
which would replace current approach: performance-analyzer-rca/src/main/java/org/opensearch/performanceanalyzer/jvm/ThreadList.java Lines 191 to 192 in c020cc9
Process' output is by default piped to an InputStream object from which we can instantly read and keep the rest of parsing process the same. Did you suggest the file writing so we can delay the reading or for some other reason, also what kind of interface would this jar you mentioned use to access the JVM info once it's attached, do we rely on some specific implementation of JVM ? |
Is your feature request related to a problem?
Right now we use HotSpotVirtualMachine class here to capture thread dump. We type cast VirtualMachine into HotSpotVirtualMachine and then try to access the method at runtime. This worked okay(with warnings) until JDK 17 came in which doesn't allow this behaviour and resulted in error.
For now we will fix this by adding "add-open" access but we should explore if we can change the logic itself to avoid runtime access of jdk internal classes/methods. As in future JDK versions, even "add-opens" practice maybe disallowed.
What solution would you like?
A clear and concise description of what you want to happen.
What alternatives have you considered?
A clear and concise description of any alternative solutions or features you've considered.
Do you have any additional context?
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: