Thanks, this is a possible patch (only slightly tested)
Code: Select all
--- a/src/search.cpp
+++ b/src/search.cpp
@@ -2578,16 +2578,12 @@ namespace {
int i, master = p.thread();
Thread& masterThread = threads[master];
- lock_grab(&MPLock);
+ // If we are here it means we are not available
+ assert(masterThread.state != THREAD_AVAILABLE);
- // If no other thread is available to help us, or if we have too many
- // active split points, don't split.
- if ( !available_thread_exists(master)
- || masterThread.activeSplitPoints >= MAX_ACTIVE_SPLIT_POINTS)
- {
- lock_release(&MPLock);
+ // If we have too many active split points, don't split
+ if (masterThread.activeSplitPoints >= MAX_ACTIVE_SPLIT_POINTS)
return;
- }
// Pick the next available split point object from the split point stack
SplitPoint& splitPoint = masterThread.splitPoints[masterThread.activeSplitPoints++];
@@ -2612,12 +2608,11 @@ namespace {
masterThread.splitPoint = &splitPoint;
- // If we are here it means we are not available
- assert(masterThread.state != THREAD_AVAILABLE);
-
int workersCnt = 1; // At least the master is included
- // Allocate available threads setting state to THREAD_BOOKED
+ lock_grab(&MPLock);
+
+ // Try to allocate available threads setting state to THREAD_BOOKED
for (i = 0; !Fake && i < ActiveThreads && workersCnt < MaxThreadsPerSplitPoint; i++)
if (thread_is_available(i, master))
{
@@ -2627,11 +2622,16 @@ namespace {
workersCnt++;
}
- assert(Fake || workersCnt > 1);
-
- // We can release the lock because slave threads are already booked and master is not available
+ // We can release the lock because slave threads are already booked
lock_release(&MPLock);
+ // If we fail to book any slave release the split point and return
+ if (!Fake && workersCnt == 1)
+ {
+ masterThread.activeSplitPoints--;
+ return;
+ }
+
// Tell the threads that they have work to do. This will make them leave
// their idle loop. But before copy search stack tail for each thread.
for (i = 0; i < ActiveThreads; i++)
While there I have also done this one to remove an useless lock
Code: Select all
--- a/src/search.cpp
+++ b/src/search.cpp
@@ -2658,14 +2658,10 @@ namespace {
// We have returned from the idle loop, which means that all threads are
// finished. Update alpha and bestValue, and return.
- lock_grab(&MPLock);
-
*alpha = splitPoint.alpha;
*bestValue = splitPoint.bestValue;
masterThread.activeSplitPoints--;
masterThread.splitPoint = splitPoint.parent;
-
- lock_release(&MPLock);
}
and especially this that seems important because fixes a possible race in case someone books us while we are waiting for other threads to finish. I don't know how was possible to miss that for so long, or perhaps I am missing something obvious. But I would like to show you so to have your comment on this:
Code: Select all
--- a/src/search.cpp
+++ b/src/search.cpp
@@ -2384,10 +2384,15 @@ namespace {
lock_grab(&(sp->lock));
lock_release(&(sp->lock));
- assert(threads[threadID].state == THREAD_AVAILABLE);
+ lock_grab(&MPLock);
- threads[threadID].state = THREAD_SEARCHING;
- return;
+ if (threads[threadID].state == THREAD_AVAILABLE)
+ {
+ threads[threadID].state = THREAD_SEARCHING;
+ lock_release(&MPLock);
+ return;
+ }
+ lock_release(&MPLock);
}
}
}
Regarding your proposal is not that we don't love, simply it doesn't seems to improve anything with a QUAD, perhas with an OCTAL we could see something better, but we don't have an OCTAL
