From f5663e41f62dd0df1c8d4aefbf0a68bfaa5c5cf5 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 13 Oct 2017 21:50:32 -0700 Subject: [PATCH 1/2] Fix hill climbing float overflow - When hill climbing finds that it wants to decrease the thread count but can't because the thread count is already the minimum, it instead tries to increase the sampling interval by a factor of up to 10 depending on how much it wanted to decrease the thread count - The ratio was being used incorrectly (used max instead of min), and sometimes the ratio can be so large that the conversion to int after the float math overflows - If something in the process enabled floating point exceptions, it may also crash - There doesn't appear to be a clean way to disable hill climbing, added a config variable that disables it in case a workaround is necessary for some other reason in the future - Fixed to avoid overflow in the math to what was probably intended - There may be another bug in GetWaveComponent() that causes values of such high magnitude to be generated, I'll leave that investigation for when that in particular becomes a real issue --- src/inc/clrconfigvalues.h | 1 + src/vm/hillclimbing.cpp | 2 +- src/vm/win32threadpool.cpp | 7 +++++++ src/vm/win32threadpool.h | 3 ++- 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/inc/clrconfigvalues.h b/src/inc/clrconfigvalues.h index 7b096b438f7b..e4274724d1d5 100644 --- a/src/inc/clrconfigvalues.h +++ b/src/inc/clrconfigvalues.h @@ -953,6 +953,7 @@ RETAIL_CONFIG_DWORD_INFO(EXTERNAL_Thread_UseAllCpuGroups, W("Thread_UseAllCpuGro CONFIG_DWORD_INFO(INTERNAL_ThreadpoolTickCountAdjustment, W("ThreadpoolTickCountAdjustment"), 0, "") +RETAIL_CONFIG_DWORD_INFO(INTERNAL_HillClimbing_Disable, W("HillClimbing_Disable"), 0, "Disables hill climbing for thread adjustments in the thread pool"); RETAIL_CONFIG_DWORD_INFO(INTERNAL_HillClimbing_WavePeriod, W("HillClimbing_WavePeriod"), 4, ""); RETAIL_CONFIG_DWORD_INFO(INTERNAL_HillClimbing_TargetSignalToNoiseRatio, W("HillClimbing_TargetSignalToNoiseRatio"), 300, ""); RETAIL_CONFIG_DWORD_INFO(INTERNAL_HillClimbing_ErrorSmoothingFactor, W("HillClimbing_ErrorSmoothingFactor"), 1, ""); diff --git a/src/vm/hillclimbing.cpp b/src/vm/hillclimbing.cpp index 598ef3e8cd61..196708a5e230 100644 --- a/src/vm/hillclimbing.cpp +++ b/src/vm/hillclimbing.cpp @@ -335,7 +335,7 @@ int HillClimbing::Update(int currentThreadCount, double sampleDuration, int numC // we'll simply stay at minThreads much longer, and only occasionally try a higher value. // if (ratio.r < 0.0 && newThreadCount == ThreadpoolMgr::MinLimitTotalWorkerThreads) - *pNewSampleInterval = (int)(0.5 + m_currentSampleInterval * (10.0 * max(-ratio.r, 1.0))); + *pNewSampleInterval = (int)(0.5 + m_currentSampleInterval * (10.0 * min(-ratio.r, 1.0))); else *pNewSampleInterval = m_currentSampleInterval; diff --git a/src/vm/win32threadpool.cpp b/src/vm/win32threadpool.cpp index 97c020a4b6eb..acf385019946 100644 --- a/src/vm/win32threadpool.cpp +++ b/src/vm/win32threadpool.cpp @@ -104,6 +104,7 @@ DWORD ThreadpoolMgr::NextCompletedWorkRequestsTime; LARGE_INTEGER ThreadpoolMgr::CurrentSampleStartTime; unsigned int ThreadpoolMgr::WorkerThreadSpinLimit; +bool ThreadpoolMgr::IsHillClimbingDisabled; int ThreadpoolMgr::ThreadAdjustmentInterval; #define INVALID_HANDLE ((HANDLE) -1) @@ -355,6 +356,7 @@ BOOL ThreadpoolMgr::Initialize() EX_TRY { WorkerThreadSpinLimit = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ThreadPool_UnfairSemaphoreSpinLimit); + IsHillClimbingDisabled = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_HillClimbing_Disable) != 0; ThreadAdjustmentInterval = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_HillClimbing_SampleIntervalLow); pADTPCount->InitResources(); @@ -426,6 +428,11 @@ BOOL ThreadpoolMgr::Initialize() forceMax = GetForceMaxWorkerThreadsValue(); MaxLimitTotalWorkerThreads = forceMax > 0 ? (LONG)forceMax : (LONG)GetDefaultMaxLimitWorkerThreads(MinLimitTotalWorkerThreads); + if (MinLimitTotalWorkerThreads == MaxLimitTotalWorkerThreads) + { + IsHillClimbingDisabled = true; + } + ThreadCounter::Counts counts; counts.NumActive = 0; counts.NumWorking = 0; diff --git a/src/vm/win32threadpool.h b/src/vm/win32threadpool.h index 764c65efdcb8..5badf013f962 100644 --- a/src/vm/win32threadpool.h +++ b/src/vm/win32threadpool.h @@ -856,7 +856,7 @@ class ThreadpoolMgr { ThreadCounter::Counts counts = WorkerCounter.GetCleanCounts(); if (counts.NumActive <= counts.MaxWorking) - return true; + return !IsHillClimbingDisabled; } return false; @@ -1020,6 +1020,7 @@ class ThreadpoolMgr static LARGE_INTEGER CurrentSampleStartTime; static unsigned int WorkerThreadSpinLimit; + static bool IsHillClimbingDisabled; static int ThreadAdjustmentInterval; SPTR_DECL(WorkRequest,WorkRequestHead); // Head of work request queue From 5ae42500ff286e0f2c8cd189337b52606582be00 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Wed, 18 Oct 2017 07:35:30 -0700 Subject: [PATCH 2/2] Address feedback --- src/vm/win32threadpool.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/vm/win32threadpool.cpp b/src/vm/win32threadpool.cpp index acf385019946..a10d08c019d1 100644 --- a/src/vm/win32threadpool.cpp +++ b/src/vm/win32threadpool.cpp @@ -428,11 +428,6 @@ BOOL ThreadpoolMgr::Initialize() forceMax = GetForceMaxWorkerThreadsValue(); MaxLimitTotalWorkerThreads = forceMax > 0 ? (LONG)forceMax : (LONG)GetDefaultMaxLimitWorkerThreads(MinLimitTotalWorkerThreads); - if (MinLimitTotalWorkerThreads == MaxLimitTotalWorkerThreads) - { - IsHillClimbingDisabled = true; - } - ThreadCounter::Counts counts; counts.NumActive = 0; counts.NumWorking = 0;