RFR(M/L, Windows): 8245042: Improve scalability of reading Windows Performance counters via PDH when using the Process object

[Daniel D. Daugherty]

On 5/14/20 8:15 PM, Markus Gronlund wrote:
> Greetings,
>
> Kindly asking for review of the following enhancement / bug fix:
>
> Enh. / bug: https://bugs.openjdk.java.net/browse/JDK-8245042
> Webrev: http://cr.openjdk.java.net/~mgronlun/8245042/webrev01/

My review is more about allocation/deallocation checking, mechanics and
HotSpot style stuff. It's been forever since I've dealt with Win* perf
counters (WinXP/WinNT days) so you'll need someone else for reviewing
whether you're using the Win APIs correctly or not...

src/hotspot/os/windows/os_perf_windows.cpp
     L1042:   assert(process_instance_count > 0, "invariant");
         So the minimum is one matching process instance? Is that an 
accurate
         requirement on the system? Or does the process that is making these
         PDH queries always count as a match?

     L1171:   if (_machine_cpu_load == NULL || 
!_machine_cpu_load->initialized) {
     L1172:     return OS_ERR;
     L1173:   }
     L1174:   assert(_machine_cpu_load != NULL, "invariant");
         Seems odd to assert this given a NULL value will return on L1172.

     L1269:   if (_context_switches == NULL || 
!_context_switches->initialized) {
     L1270:     return OS_ERR;
     L1271:   }
     L1272:   assert(_context_switches != NULL, "invariant");
         Seems odd to assert this given a NULL value will return on L1270.

     old L1341:     _cpu_info = NULL;
         This deletion of clearing the _cpu_info field caught my eye.
         Any particular reason for deleting that line?

         The style in this code appears to be clearing the field after
         dealloc so I'm surprised here.

src/hotspot/os/windows/pdh_interface.hpp
     No comments.

src/hotspot/os/windows/pdh_interface.cpp
     No comments.


Thumbs up!

> The Jira issue contains a (very) long problem description.

Outstanding problem description.

Dan

>
> Thanks in advance
> Markus