Add PDH counter lookup and enumeration functions #973
Conversation
|
I have no idea why the diffs think I'm changing all the lines when I'm just adding to existing. Probably something to do with line endings. Will try to fix that once I figure how... |
| */ | ||
| int PdhEnumObjectItems(String szDataSource, String szMachineName, String szObjectName, char[] mszCounterList, | ||
| DWORDByReference pcchCounterListLength, char[] mszInstanceList, DWORDByReference pcchInstanceListLength, | ||
| int dwDetailLevel, int dwFlags); |
There was a problem hiding this comment.
The char[] parameters are problematic, as the type mapper will not change their mapping. A java char will be mapped to a wchar on the C side, but if w32.ascii is in effect, not LPWSTR is required (this is a wchar[]), but a LPSTR' (a char[]`).
So what was done in other places was this: map the function with a byte[] as parameter and add a <Lib>Util class, that hold a helper function, that unwraps the value correctly.
There was a problem hiding this comment.
Is there another example in the JNA project where this was done such that the Util class already exists?
There was a problem hiding this comment.
I think this is a good sample:
Here is the MSDN definition
https://msdn.microsoft.com/en-us/library/windows/desktop/aa376556(v=vs.85).aspx
Binding
https://github.com/java-native-access/jna/blob/master/contrib/platform/src/com/sun/jna/platform/win32/Crypt32.java#L359
UtilHelper
https://github.com/java-native-access/jna/blob/master/contrib/platform/src/com/sun/jna/platform/win32/Crypt32Util.java#L195
psz is an LPTSTR so it depends on which function variant is called (CertNameToStrW => unicode, CertNameToStrA => ascii).
There was a problem hiding this comment.
I think there are also some instances which explicitly declare both the -A and -W of a function. The library invocation handler for W32API automatically tries appending the suffix if the explicit name does not exist, but if the explicit name is used, that's what you'll get. Then you can wrap those and choose in a util wrapper.
| * error code. | ||
| */ | ||
| int PdhLookupPerfNameByIndex(String szMachineName, int dwNameIndex, char[] szNameBuffer, | ||
| DWORDByReference pcchNameBufferSize); |
There was a problem hiding this comment.
Here the same as above applies.
|
|
||
| @Test | ||
| public void testLookupPerfIndex() { | ||
| // Index 238 is "Processor" |
There was a problem hiding this comment.
The tests are locale specific (it failed on my initial run on a german locale). Please guard the critical parts with AbstractWin32TestSupport.isEnglishLocale. See here for an example:
|
RE: the test failures. We're still building for Java 6?? :) |
| List<String> counters = Native.toStringList(mszCounterList); | ||
| assertTrue(counters.contains(processorTimeStr)); | ||
| if (AbstractWin32TestSupport.isEnglishLocale) { | ||
| String processorStr = "Process"; |
There was a problem hiding this comment.
Is this tested? I tested with a german locale and the translation of "Process" to german "Prozess" gave me performance counters concerning processes on the machine, not the processor. When I switched to "Prozessor" (I would translate it as "processor") (the CPU), I got the expected values for the single processor I expose via VirtualBox and "_Total" . This also does not match the original code.
There was a problem hiding this comment.
Well, it was tested before I fiddled with the String to debug something else and forgot to put it back. D'oh.
| * Strings of the counters for the object. Index 1 contains a List | ||
| * of Strings of the instances of the object. | ||
| */ | ||
| public static List<List<String>> PdhEnumObjectItems(String szDataSource, String szMachineName, String szObjectName, |
There was a problem hiding this comment.
I would think about splitting this method - is it performance critical? If not, I think two method (one for the conters and one for the instances would be better), the List of List of String feels strange.
There was a problem hiding this comment.
I had that same thought. In my application I only am using the instance list anyway. The List of lists is a step up from my original implementation as an array of lists, though, so give me partial credit.
| pcchCounterListLength, mszInstanceList, pcchInstanceListLength, dwDetailLevel, 0); | ||
|
|
||
| // Fetch counters | ||
| List<String> counters = new LinkedList<>(); |
There was a problem hiding this comment.
In another issue it was noted, that the LinkedList is not the best list to choose. I suggest to switch to ArrayList.
There was a problem hiding this comment.
Sure, I'll switch. I'm curious the logic, though? I thought LinkedList was faster for adding (particularly when the final length is unknown) and ArrayList for getting.
|
Thanks for the update - one last nitpick: Could you please check the format in the files PdhUtil for example starts good (indent with 4 spaces), but lower down it looks mixed. And as you are already editting, please add the Utility methods also the CHANGE.log. Other than that, this looks merge ready. Thanks for working through it. |
|
Is there a standard formatting convention that the JNA project uses? I'd love to try to set up Eclipse to format properly, and unfortunately turning off "format the whole file" and choosing only to format edited lines ends up sometimes missing things when I move them around.... but if I can configure to format the whole file, it'd be easier. |
|
I'm not aware of a general coding convention. Thank you for the update though - the unittests run through and I'll merge this in. |
|
Looks like the one I had is good enough except I forgot to reconfigure the dang tabs vs. spaces parameter. I missed updating the Pdh.java indents, but will be submitting another PR soon so I'll just include it with that. |
No description provided.