Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix hill climbing float overflow #14505

Merged
merged 2 commits into from
Oct 19, 2017
Merged

Fix hill climbing float overflow #14505

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5663e4
Fix hill climbing float overflow
kouvel Oct 14, 2017
5ae4250
Address feedback
kouvel Oct 18, 2017
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
1 change: 1 addition & 0 deletions src/inc/clrconfigvalues.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "");
Expand Down
2 changes: 1 addition & 1 deletion src/vm/hillclimbing.cpp
View file
Open in desktop
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)));
Copy link
Member

Choose a reason for hiding this comment

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

In addition to the overflow concerns, what kind of impact was the max vs min having on actual thread growth? I'm wondering if this might actually have substantial implications and need some broader testing on a variety of scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thread growth calculation was doing the proper min/max above this code to limit ratio.r to {-1..1}. An effect of this issue is that the new sample interval becomes int32_min after overflow (exception on overflow is disabled by default) and it's added to a GetTickCount() value to determine the next threshold for sampling (stored as unsigned), so the interval becomes very large and hill climbing gets disabled.

Copy link
Member

Choose a reason for hiding this comment

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

The thread growth calculation was doing the proper min/max above this code to limit ratio.r to {-1..1}.

Ah. Ok.

else
*pNewSampleInterval = m_currentSampleInterval;

Expand Down
7 changes: 7 additions & 0 deletions src/vm/win32threadpool.cpp
View file
Open in desktop
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 Expand Up @@ -426,6 +428,11 @@ BOOL ThreadpoolMgr::Initialize()
forceMax = GetForceMaxWorkerThreadsValue();
MaxLimitTotalWorkerThreads = forceMax > 0 ? (LONG)forceMax : (LONG)GetDefaultMaxLimitWorkerThreads(MinLimitTotalWorkerThreads);

if (MinLimitTotalWorkerThreads == MaxLimitTotalWorkerThreads)
{
IsHillClimbingDisabled = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

What if the min and max are changed by the dev with calls to ThreadPool.SetMin/MaxThreads. Could we go from a situation where hill climbing is disabled to one where it shouldn't be?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, this is unnecessary as well, I'll remove it


ThreadCounter::Counts counts;
counts.NumActive = 0;
counts.NumWorking = 0;
Expand Down
3 changes: 2 additions & 1 deletion src/vm/win32threadpool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,7 @@ class ThreadpoolMgr
{
ThreadCounter::Counts counts = WorkerCounter.GetCleanCounts();
if (counts.NumActive <= counts.MaxWorking)
return true;
return !IsHillClimbingDisabled;
}

return false;
Expand Down Expand Up @@ -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
Expand Down