MP Stockfish

Discussion of chess software programming and technical issues.

Moderator: Ras

hcyrano

MP Stockfish

Post by hcyrano »

hi,

in void ThreadsManager::idle_loop(int threadID, SplitPoint* sp);

can you explain this lines:


// Because sp->slaves[] is reset under lock protection,
// be sure sp->lock has been released before to return.
lock_grab(&(sp->lock));
lock_release(&(sp->lock));

i don't see real utility :-(

thx a lot
hcyrano

Re: MP Stockfish

Post by hcyrano »

ok i see :D

return a object Splitpoint clean.

@+
mcostalba
Posts: 2684
Joined: Sat Jun 14, 2008 9:17 pm

Re: MP Stockfish

Post by mcostalba »

hcyrano wrote:ok i see :D

return a object Splitpoint clean.

@+
Sorry for late reply, I have seen your question, but I wanted to answer during the week end because is not a trivial question and I want to have a bit of time....more later !
zamar
Posts: 613
Joined: Sun Jan 18, 2009 7:03 am

Re: MP Stockfish

Post by zamar »

hcyrano wrote:hi,

in void ThreadsManager::idle_loop(int threadID, SplitPoint* sp);

can you explain this lines:


// Because sp->slaves[] is reset under lock protection,
// be sure sp->lock has been released before to return.
lock_grab(&(sp->lock));
lock_release(&(sp->lock));

i don't see real utility :-(

thx a lot
If I remember correctly, I came to conclusion that this could be safely removed. For some reason the change never entered the codebase...

The point is of course to have clean splitPoint object, but I think I came to conclusion that this isn't absolute necessity.
Joona Kiiski
hcyrano

Re: MP Stockfish

Post by hcyrano »

.
the point is of course to have clean splitPoint object, but I think I came to conclusion that this isn't absolute necessity.
I agree, but I think it is useful to have an object splitPoint clean after using.

this is more satisfying, keep this code.

for info: i prefere "old" code this nCpus :D, because i can use std::vector<int> slaves and not a fix table volatile int slaves[].
mcostalba
Posts: 2684
Joined: Sat Jun 14, 2008 9:17 pm

Re: MP Stockfish

Post by mcostalba »

hcyrano wrote:.
the point is of course to have clean splitPoint object, but I think I came to conclusion that this isn't absolute necessity.
I agree, but I think it is useful to have an object splitPoint clean after using.

this is more satisfying, keep this code.

for info: i prefere "old" code this nCpus :D, because i can use std::vector<int> slaves and not a fix table volatile int slaves[].
Yes but you have an additional shared variable. Here the aim is to reduce the shared vairables to the bare minimum to keep things easier.

A shared variable is a source of possible races especially ncpu that was used to sync the threads to leave the split point.

Anyhow please feel free to send a patch according to your idea and if it is better then current code we will apply.
hcyrano

Re: MP Stockfish

Post by hcyrano »

hi,

i think you can reduce the protected region in ThreadsManager::split at this simple code:


pseudo code :wink:

Code: Select all

 
    // Pick the next available split point object from the split point stack
    SplitPoint& splitPoint = masterThread.splitPoints[masterThread.activeSplitPoints];

    int workersCnt = 1; // At least the master is included

    lock_grab(&MPLock);


    // Allocate available threads setting state to THREAD_BOOKED
    for (unsigned int i = 0; !Fake && i < ActiveThreads; i++) {
    
    	splitPoint.slaves[i] = 0;
    	
        if (workersCnt < MaxThreadsPerSplitPoint && thread_is_available(i, master))
        {
            threads[i].state = THREAD_BOOKED;
            threads[i].splitPoint = &splitPoint; //initialisation later
            splitPoint.slaves[i] = 1;
            workersCnt++;
        }
   }
       
    // We can release the lock because slave threads are already booked and master is not available
    lock_release(&MPLock);

    if (workerCnt == 1)
    	return;
    	
    masterThread.activeSplitPoints++;

point 2 :

you don't love my "helpful master" concept recursif ?


Code: Select all

	for(RXSplitPoint* sp = threads[master].splitPoint; sp != NULL; sp = sp->parent) {
		if(sp == &(threads[slave].splitPointStack[localActiveSplitPoints-1]))
			return true;
	}
mcostalba
Posts: 2684
Joined: Sat Jun 14, 2008 9:17 pm

Re: MP Stockfish

Post by mcostalba »

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 :-(
hcyrano

Re: MP Stockfish

Post by hcyrano »

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:
helpful master concept : can help only a sub-tree, or here all is finish, so it is not possible taht this thread are booked.
hcyrano

Re: MP Stockfish

Post by hcyrano »

mcostalba wrote:Thanks, this is a possible patch (only slightly tested)


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 :-(
New macpro can have 24 threads (12 + 12HT) :D