Skip to content

Commit

Permalink
Fix hill climbing float overflow (dotnet/coreclr#14505)
Browse files Browse the repository at this point in the history
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

Commit migrated from dotnet/coreclr@a589e39
  • Loading branch information
kouvel authored Oct 19, 2017
1 parent caf173c commit 4497052
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 2 deletions.
1 change: 1 addition & 0 deletions src/coreclr/src/inc/clrconfigvalues.h
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,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, "");
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/vm/hillclimbing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/vm/win32threadpool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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();
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/src/vm/win32threadpool.h
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ class ThreadpoolMgr
{
ThreadCounter::Counts counts = WorkerCounter.GetCleanCounts();
if (counts.NumActive <= counts.MaxWorking)
return true;
return !IsHillClimbingDisabled;
}

return false;
Expand Down Expand Up @@ -1025,6 +1025,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
Expand Down

0 comments on commit 4497052

Please sign in to comment.