RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager
Ioi Lam
iklam at openjdk.java.net
Tue Oct 19 21:45:07 UTC 2021
On Tue, 19 Oct 2021 21:17:57 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:
>> LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs owned by a specific user. No one uses these APIs, and they don't work anyway.
>>
>> The current code is very confusing to look at. Since we're likely to change code in this area for further container support, it's better to clean up the code now.
>>
>> - Remove all APIs that take a user name
>> - Also removed PerfDataFile.getFile() methods that are unused
>> - Cleaned up the code that looks up the hsperfdata_xxx files
>> - Fix comments to explain what's happening
>> - Avoid using Matcher.reset which is not thread-safe
>> - Renamed confusing variables such as `userFilter` to make the code more readable
>> - LocalVmManager.activeVms() probably doesn't need to be synchronized, but I kept it anyway to avoid unnecessary risks.
>>
>> Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers and have extensive use of the management tools).
>
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataFile.java line 80:
>
>> 78: "^hsperfdata_[0-9]+(_[1-2]+)?$";
>> 79:
>> 80:
>
> I don't understand why you thought it best to remove these and instead use hard coded references to these partterns.
I have two goals
- these aren't used anywhere else, so it's better to at least move them from this file to LocalVmManager.java, where they are actually used. So you don't need to flip between two files.
- the behavior is easier to understand if the string literals, comments, and the code that uses them are together.
Note that the original code has plenty of comment that are rather useless if you want to know how the files are searched.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5923
More information about the serviceability-dev
mailing list