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

[Markus Gronlund]

Hi Dan,

Thank you very much for taking the time to have a look, appreciated.

Pls see inline.

Cheers
Markus

-----Original Message-----
From: Daniel D. Daugherty 
Sent: den 18 maj 2020 21:24
To: Markus Gronlund <markus.gronlund at oracle.com>; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(M/L, Windows): 8245042: Improve scalability of reading Windows Performance counters via PDH when using the Process object

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?

[MG] Yes, process instance count includes this process. 

     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.

[MG] I agree, thank you for noticing.

     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.

[MG] I agree, thank you again for noticing.

     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.

[MG] No reason besides this code now (compared to many years ago) being located in a proper C++ destructor. Think I wanted to clean up some of the things I put in with the original. I will either not touch it our if I do, I will do the same for the iterator field of the SystemProcess destructor too, thanks.

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