Horror code

Discussion of chess software programming and technical issues.

Moderators: hgm, Rebel, chrisw

Henk
Posts: 7220
Joined: Mon May 27, 2013 10:31 am

Horror code

Post by Henk »

When user selects a position my engine throws an exception when computing pv's for a selected position. Unfortunately this does not happen when playing games (single threaded).

This is probably because my code is not thread safe for it starts a new thread when computing pv's and also the view (helper) creates a new factory when it refreshes the screen.

Code: Select all

 
    public PositionFactory()
        {
            lock(this)
            {
                ChessBoardBits.InitIndex();
            }
            lock (this)
            {
                ChessBoardBits.InitIndex2();
            }
        ..

   public class ChessBoardBits : IChessBoardBits
    {
       static int[] index;
        static int[] index2;


     public static void InitIndex()
        {
           
            index = new int[64];
            
            index[0]=0;   index[1]=1;   index[2]=2;   index[4]=3;   index[8]=4;   index[17]=5;  index[34]=6;  index[5]=7;
            index[11]=8;  index[23]=9;  index[47]=10; index[31]=11; index[63]=12; index[62]=13; index[61]=14; index[59]=15;
            index[55]=16; index[46]=17; index[29]=18; index[58]=19; index[53]=20; index[43]=21; index[22]=22; index[44]=23;
            index[24]=24; index[49]=25; index[35]=26; index[7]=27;  index[15]=28; index[30]=29; index[60]=30; index[57]=31;
            index[51]=32; index[38]=33; index[12]=34; index[25]=35; index[50]=36; index[36]=37; index[9]=38;  index[18]=39;
            index[37]=40; index[10]=41; index[21]=42; index[42]=43; index[20]=44; index[41]=45; index[19]=46; index[39]=47;
            index[14]=48; index[28]=49; index[56]=50; index[48]=51; index[33]=52; index[3]=53;  index[6]=54;  index[13]=55;
            index[27]=56; index[54]=57; index[45]=58; index[26]=59; index[52]=60; index[40]=61; index[16]=62; index[32]=63;


            for &#40;int i = 0; i <= 63; i++)
            &#123;
                int k = index&#91;i&#93;;
                int rowNr = ChessBoard.FIRSTROW + Row&#40;k&#41;;
                int colNr = ChessBoard.FIRSTCOLUMN + Col&#40;k&#41;;
                index&#91;i&#93; = &#40;rowNr - 1&#41; * ChessBoard.NCOLS + colNr - 1;
            &#125;
        &#125;

        public static void InitIndex2&#40;)
        &#123;

            index2 = new int&#91;64&#93;;

            index2&#91;0&#93; = 0; index2&#91;1&#93; = 1; index2&#91;2&#93; = 2; index2&#91;4&#93; = 3; index2&#91;8&#93; = 4; index2&#91;17&#93; = 5; index2&#91;34&#93; = 6; index2&#91;5&#93; = 7;
            index2&#91;11&#93; = 8; index2&#91;23&#93; = 9; index2&#91;47&#93; = 10; index2&#91;31&#93; = 11; index2&#91;63&#93; = 12; index2&#91;62&#93; = 13; index2&#91;61&#93; = 14; index2&#91;59&#93; = 15;
            index2&#91;55&#93; = 16; index2&#91;46&#93; = 17; index2&#91;29&#93; = 18; index2&#91;58&#93; = 19; index2&#91;53&#93; = 20; index2&#91;43&#93; = 21; index2&#91;22&#93; = 22; index2&#91;44&#93; = 23;
            index2&#91;24&#93; = 24; index2&#91;49&#93; = 25; index2&#91;35&#93; = 26; index2&#91;7&#93; = 27; index2&#91;15&#93; = 28; index2&#91;30&#93; = 29; index2&#91;60&#93; = 30; index2&#91;57&#93; = 31;
            index2&#91;51&#93; = 32; index2&#91;38&#93; = 33; index2&#91;12&#93; = 34; index2&#91;25&#93; = 35; index2&#91;50&#93; = 36; index2&#91;36&#93; = 37; index2&#91;9&#93; = 38; index2&#91;18&#93; = 39;
            index2&#91;37&#93; = 40; index2&#91;10&#93; = 41; index2&#91;21&#93; = 42; index2&#91;42&#93; = 43; index2&#91;20&#93; = 44; index2&#91;41&#93; = 45; index2&#91;19&#93; = 46; index2&#91;39&#93; = 47;
            index2&#91;14&#93; = 48; index2&#91;28&#93; = 49; index2&#91;56&#93; = 50; index2&#91;48&#93; = 51; index2&#91;33&#93; = 52; index2&#91;3&#93; = 53; index2&#91;6&#93; = 54; index2&#91;13&#93; = 55;
            index2&#91;27&#93; = 56; index2&#91;54&#93; = 57; index2&#91;45&#93; = 58; index2&#91;26&#93; = 59; index2&#91;52&#93; = 60; index2&#91;40&#93; = 61; index2&#91;16&#93; = 62; index2&#91;32&#93; = 63;


        &#125;

     public static int Index&#40;ulong bb&#41;
        &#123;
           
            return index&#91;&#40;bb * debruijn64&#41; >> 58&#93;;
        &#125;

        public static int Index2&#40;ulong bb&#41;
        &#123;
            int i = index2&#91;&#40;bb * debruijn64&#41; >> 58&#93;;
            int rowNr = ChessBoard.FIRSTROW + Row&#40;i&#41;;
            int colNr = ChessBoard.FIRSTCOLUMN + Col&#40;i&#41;;
            int j = &#40;rowNr - 1&#41; * ChessBoard.NCOLS + colNr - 1;
            return j;
        &#125;


       public static Field GetField&#40;IBoardSquares chessBoard, ulong bitCoord&#41;
        &#123;
            int i = Index&#40;bitCoord&#41;;
            int j = Index2&#40;bitCoord&#41;;
            if &#40;i != j&#41;
                throw new Exception&#40;"error in getfield 2");

      ..

I think I have to implement a thread safe index. Using thread safe singleton pattern.

If I would use index2 there would be no problem but it is less efficient.
Henk
Posts: 7220
Joined: Mon May 27, 2013 10:31 am

Re: Horror code

Post by Henk »

Actually I still don't understand why it goes wrong. Problem already took me four hours.
jdart
Posts: 4367
Joined: Fri Mar 10, 2006 5:23 am
Location: http://www.arasanchess.org

Re: Horror code

Post by jdart »

for a "thread safe index" have a look at the "std::atomic" feature that is in the C++ 11 library (http://en.cppreference.com/w/cpp/atomic/atomic).
Henk
Posts: 7220
Joined: Mon May 27, 2013 10:31 am

Re: Horror code

Post by Henk »

I use C#.net.

I don't understand what in the given code is the cause of the bug. So it also might have nothing to do with thread safe code. Perhaps uninitialized data (?). But the given code is executed a million times when playing single threaded games and it never gives an exception.


I can also eliminate code below. Put the result of this code directly in the index. But I don't think that's right.

Code: Select all

 for &#40;int i = 0; i <= 63; i++) 
 &#123; 
                 int k = index&#91;i&#93;; 
                 int rowNr = ChessBoard.FIRSTROW + Row&#40;k&#41;; 
                 int colNr = ChessBoard.FIRSTCOLUMN + Col&#40;k&#41;; 
                 index&#91;i&#93; = &#40;rowNr - 1&#41; * ChessBoard.NCOLS + colNr - 1; 
   &#125; 
I can look up the C# code for thread safe singleton pattern. No problem.
User avatar
stegemma
Posts: 859
Joined: Mon Aug 10, 2009 10:05 pm
Location: Italy
Full name: Stefano Gemma

Re: Horror code

Post by stegemma »

Henk wrote:I use C#.net.

I don't understand what in the given code is the cause of the bug. So it also might have nothing to do with thread safe code. Perhaps uninitialized data (?). But the given code is executed a million times when playing single threaded games and it never gives an exception.


I can also eliminate code below. Put the result of this code directly in the index. But I don't think that's right.

Code: Select all

 for &#40;int i = 0; i <= 63; i++) 
 &#123; 
                 int k = index&#91;i&#93;; 
                 int rowNr = ChessBoard.FIRSTROW + Row&#40;k&#41;; 
                 int colNr = ChessBoard.FIRSTCOLUMN + Col&#40;k&#41;; 
                 index&#91;i&#93; = &#40;rowNr - 1&#41; * ChessBoard.NCOLS + colNr - 1; 
   &#125; 
I can look up the C# code for thread safe singleton pattern. No problem.
Maybe you should add some assert on validity of k, rowNr and colNr, just to break the program before it raise the exception.
Author of Drago, Raffaela, Freccia, Satana, Sabrina.
http://www.linformatica.com
Sven
Posts: 4052
Joined: Thu May 15, 2008 9:57 pm
Location: Berlin, Germany
Full name: Sven Schüle

Re: Horror code

Post by Sven »

Henk wrote:and also the view (helper) creates a new factory when it refreshes the screen
Henk wrote:I don't understand what in the given code is the cause of the bug.
Questions:

1) Which kind of exception do you get? What lets you know it is related to the "index" stuff?

2) What do you mean by "creates a new factory"? Usually a factory is something you use to create something else, not something that itself will be created.

3) Which class would be your candidate to make it a "thread safe singleton"? Do you mean "ChessBoardBits"? But currently that isn't even a singleton, it is just something with static data and static methods.

Apart from that, I think you need to ensure that your index tables exist just once in memory, and are properly initialized before being used for the first time. It's simple :-)
Henk
Posts: 7220
Joined: Mon May 27, 2013 10:31 am

Re: Horror code

Post by Henk »

2) I like to have the methods that handle a request to be stateless. If there is no factory (created) nothing can be created.

The more state (variables) the more misery.
Sven
Posts: 4052
Joined: Thu May 15, 2008 9:57 pm
Location: Berlin, Germany
Full name: Sven Schüle

Re: Horror code

Post by Sven »

Henk wrote:2) I like to have the methods that handle a request to be stateless. If there is no factory (created) nothing can be created.

The more state (variables) the more misery.
But the factory itself will not be "created", it "creates" something else.

I get the impression that you allocate a new piece of memory for your "index" table(s) each time you call PositionFactory(), since InitIndex() does a "new int[64]". If that is the case then I also guess that is not what you intended. Therefore you do not need a "factory" to "create" something that is more like a singleton, for that purpose you normally use an appropriate "getInstance()" method or similar of the singleton class itself. Typically you use a factory to "produce" something that should be produced as often as it is needed.

It is possible that your implementation does most things right regarding all of the above. But in my experience using "unusual" terms often causes trouble.
Henk
Posts: 7220
Joined: Mon May 27, 2013 10:31 am

Re: Horror code

Post by Henk »

Smallest change first. Looks like bug has gone. But my engine has more of these non thread safe static variables. Ok position factory should be singleton too.

Code: Select all

 
public static Field GetField&#40;IBoardSquares chessBoard, ulong bitCoord&#41;
&#123;
            int i = BitBoardIndex.Instance.Index&#40;bitCoord&#41;;
            var field = chessBoard&#91;i&#93;;
            return field;
&#125;
Henk
Posts: 7220
Joined: Mon May 27, 2013 10:31 am

Re: Horror code

Post by Henk »

Henk wrote:Smallest change first. Looks like bug has gone. But my engine has more of these non thread safe static variables. Ok position factory should be singleton too.

Code: Select all

 
public static Field GetField&#40;IBoardSquares chessBoard, ulong bitCoord&#41;
&#123;
            int i = BitBoardIndex.Instance.Index&#40;bitCoord&#41;;
            var field = chessBoard&#91;i&#93;;
            return field;
&#125;
Better:

Code: Select all


    public class ChessBoard&#58; IBoardSquares
    &#123;
..
        public Field GetField&#40; ulong bitCoord&#41;
        &#123;
            int i = BitBoardIndex.Instance.Index&#40;bitCoord&#41;;
            var field = this&#91;i&#93;;

            Debug.Assert&#40;field.RowNr >= FIRSTROW&#41;;
            Debug.Assert&#40;field.ColNr >= FIRSTCOLUMN&#41;;
            Debug.Assert&#40;field.RowNr <= LASTROW&#41;;
            Debug.Assert&#40;field.ColNr <= LASTCOLUMN&#41;;

            return field;
        &#125;
..