Skip to content

CPU utilization computation fixes #2196

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.

Sign up for GitHub

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

Merged
jkotas merged 2 commits into from
Jan 25, 2020
Merged

CPU utilization computation fixes #2196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6de80e2
CPU utilization computation fixes
jkotas Jan 25, 2020
2fff9a8
Fix buildbreak
jkotas Jan 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions ...c/System.Private.CoreLib/src/System/Diagnostics/Eventing/RuntimeEventSourceHelper.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@ namespace System.Diagnostics.Tracing
{
internal static class RuntimeEventSourceHelper
{
private static Interop.Sys.ProcessCpuInformation cpuInfo;
private static Interop.Sys.ProcessCpuInformation s_cpuInfo;

internal static int GetCpuUsage()
{
return Interop.Sys.GetCpuUtilization(ref cpuInfo);
}
internal static int GetCpuUsage() =>
Interop.Sys.GetCpuUtilization(ref s_cpuInfo) / Environment.ProcessorCount;
}
}
22 changes: 11 additions & 11 deletions ...ystem.Private.CoreLib/src/System/Diagnostics/Eventing/RuntimeEventSourceHelper.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ namespace System.Diagnostics.Tracing
{
internal static class RuntimeEventSourceHelper
{
private static long prevProcUserTime = 0;
private static long prevProcKernelTime = 0;
private static long prevSystemUserTime = 0;
private static long prevSystemKernelTime = 0;
private static long s_prevProcUserTime = 0;
private static long s_prevProcKernelTime = 0;
private static long s_prevSystemUserTime = 0;
private static long s_prevSystemKernelTime = 0;

internal static int GetCpuUsage()
{
Expand All @@ -27,21 +27,21 @@ internal static int GetCpuUsage()
return 0;
}

if (prevSystemUserTime == 0 && prevSystemKernelTime == 0) // These may be 0 when we report CPU usage for the first time, in which case we should just return 0.
if (s_prevSystemUserTime == 0 && s_prevSystemKernelTime == 0) // These may be 0 when we report CPU usage for the first time, in which case we should just return 0.
{
cpuUsage = 0;
}
else
{
long totalProcTime = (procUserTime - prevProcUserTime) + (procKernelTime - prevProcKernelTime);
long totalSystemTime = (systemUserTime - prevSystemUserTime) + (systemKernelTime - prevSystemKernelTime);
long totalProcTime = (procUserTime - s_prevProcUserTime) + (procKernelTime - s_prevProcKernelTime);
long totalSystemTime = (systemUserTime - s_prevSystemUserTime) + (systemKernelTime - s_prevSystemKernelTime);
cpuUsage = (int)(totalProcTime * 100 / totalSystemTime);
}

prevProcUserTime = procUserTime;
prevProcKernelTime = procKernelTime;
prevSystemUserTime = systemUserTime;
prevSystemKernelTime = systemKernelTime;
s_prevProcUserTime = procUserTime;
s_prevProcKernelTime = procKernelTime;
s_prevSystemUserTime = systemUserTime;
s_prevSystemKernelTime = systemKernelTime;

return cpuUsage;
}
Expand Down
23 changes: 0 additions & 23 deletions src/libraries/Native/Unix/System.Native/pal_time.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,25 +93,8 @@ uint64_t SystemNative_GetTimestamp()
#endif
}

#if defined(_ARM_) || defined(_ARM64_)
#define SYSCONF_GET_NUMPROCS _SC_NPROCESSORS_CONF
#else
#define SYSCONF_GET_NUMPROCS _SC_NPROCESSORS_ONLN
#endif

int32_t SystemNative_GetCpuUtilization(ProcessCpuInformation* previousCpuInfo)
{
static long numProcessors = 0;

if (numProcessors <= 0)
{
numProcessors = sysconf(SYSCONF_GET_NUMPROCS);
if (numProcessors <= 0)
{
return 0;
}
}

uint64_t kernelTime = 0;
uint64_t userTime = 0;

Expand Down Expand Up @@ -143,11 +126,7 @@ int32_t SystemNative_GetCpuUtilization(ProcessCpuInformation* previousCpuInfo)
uint64_t cpuTotalTime = 0;
if (currentTime > lastRecordedCurrentTime)
{
// cpuTotalTime is based on clock time. Since multiple threads can run in parallel,
// we need to scale cpuTotalTime cover the same amount of total CPU time.
// rusage time is already scaled across multiple processors.
cpuTotalTime = (currentTime - lastRecordedCurrentTime);
cpuTotalTime *= (uint64_t)numProcessors;
}

uint64_t cpuBusyTime = 0;
Expand All @@ -162,8 +141,6 @@ int32_t SystemNative_GetCpuUtilization(ProcessCpuInformation* previousCpuInfo)
cpuUtilization = (int32_t)(cpuBusyTime * 100 / cpuTotalTime);
}

assert(cpuUtilization >= 0 && cpuUtilization <= 100);

previousCpuInfo->lastRecordedCurrentTime = currentTime;
previousCpuInfo->lastRecordedUserTime = userTime;
previousCpuInfo->lastRecordedKernelTime = kernelTime;
Expand Down
4 changes: 3 additions & 1 deletion src/libraries/Native/Unix/System.Native/pal_time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ DLLEXPORT uint64_t SystemNative_GetTimestamp(void);
* for the CLR thread pool to regulate the number of worker threads.
* Since there is no consistent API on Unix to get the CPU utilization
* from a user process, getrusage and gettimeofday are used to
* compute the current process's CPU utilization instead.
* compute the current process's CPU utilization instead. The CPU utilization
* returned is sum of utilization accross all processors, e.g. this function will
* return 200 when two cores are running at 100%.
Copy link
Copy Markdown
Member

@stephentoub stephentoub Jan 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by removing the multiplication by number of processors, but then this saying it's larger for more processors. I'm sure you're right, but what's my misunderstanding?

Copy link
Copy Markdown
Member Author

@jkotas jkotas Jan 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.Native shim does not have easy access to the process count adjusted for container limits. The processor count on managed size is adjusted and thus it is the right number to use to compute how much spare cycles may extra threads take advantage of.

In other words, this change should get us equivalent of this piece of logic from CoreCLR PAL.

Copy link
Copy Markdown
Member

@filipnavara filipnavara Jan 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's essentially removing division, not multiplication. It's not obvious from reading the unified diff because GitHub hides the cpuUtilization = (int32_t)(cpuBusyTime * 100 / cpuTotalTime); line. The multiplication in cpuTotalTime is affecting the divisor.

Copy link
Copy Markdown
Member

@stephentoub stephentoub Jan 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, I missed that, thanks.

*/
DLLEXPORT int32_t SystemNative_GetCpuUtilization(ProcessCpuInformation* previousCpuInfo);
5 changes: 3 additions & 2 deletions ...stem.Private.CoreLib/src/System/Threading/PortableThreadPool.CpuUtilizationReader.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ namespace System.Threading
{
internal partial class PortableThreadPool
{
private class CpuUtilizationReader
private struct CpuUtilizationReader
{
private Interop.Sys.ProcessCpuInformation _cpuInfo;

public int CurrentUtilization => Interop.Sys.GetCpuUtilization(ref _cpuInfo); // Updates cpuInfo as a side effect for the next call
public int CurrentUtilization =>
Interop.Sys.GetCpuUtilization(ref _cpuInfo) / Environment.ProcessorCount;
}
}
}
28 changes: 10 additions & 18 deletions ...m.Private.CoreLib/src/System/Threading/PortableThreadPool.CpuUtilizationReader.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,35 +9,27 @@ namespace System.Threading
{
internal partial class PortableThreadPool
{
private class CpuUtilizationReader
private struct CpuUtilizationReader
{
private struct ProcessCpuInformation
{
public long idleTime;
public long kernelTime;
public long userTime;
}

private ProcessCpuInformation _processCpuInfo = new ProcessCpuInformation();
public long _idleTime;
public long _kernelTime;
public long _userTime;

public int CurrentUtilization
{
get
{
if (!Interop.Kernel32.GetSystemTimes(out var idleTime, out var kernelTime, out var userTime))
{
int error = Marshal.GetLastWin32Error();
var exception = new OutOfMemoryException();
exception.HResult = error;
throw exception;
return 0;
}

long cpuTotalTime = ((long)userTime - _processCpuInfo.userTime) + ((long)kernelTime - _processCpuInfo.kernelTime);
long cpuBusyTime = cpuTotalTime - ((long)idleTime - _processCpuInfo.idleTime);
long cpuTotalTime = ((long)userTime - _userTime) + ((long)kernelTime - _kernelTime);
long cpuBusyTime = cpuTotalTime - ((long)idleTime - _idleTime);
Copy link
Copy Markdown
Member

@stephentoub stephentoub Jan 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any tests that would fall if we got something wrong here?

Copy link
Copy Markdown
Member Author

@jkotas jkotas Jan 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, this is only used for Mono on Windows that gets a very light testing. If we got this wrong, this would manifest itself as a performance bug in thread pool scaling.

Hopefully, we will find time to switch to the managed threadpool for CoreCLR too. Validating details like this would be the bulk of the work for that.


_processCpuInfo.kernelTime = (long)kernelTime;
_processCpuInfo.userTime = (long)userTime;
_processCpuInfo.idleTime = (long)idleTime;
_kernelTime = (long)kernelTime;
_userTime = (long)userTime;
_idleTime = (long)idleTime;

if (cpuTotalTime > 0 && cpuBusyTime > 0)
{
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.GateThread.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ private static class GateThread

private static LowLevelLock s_createdLock = new LowLevelLock();

private static readonly CpuUtilizationReader s_cpu = new CpuUtilizationReader();
private static CpuUtilizationReader s_cpu = new CpuUtilizationReader();
private const int MaxRuns = 2;

// TODO: CoreCLR: Worker Tracking in CoreCLR? (Config name: ThreadPool_EnableWorkerTracking)
Expand Down