O wait I forgot a !. That explains the performance difference.
So code I wrote yersterday was not correct.
stage == 3 && allowStandPat && !p.position.GivesCheck(mv)
Simplifying code
Moderators: hgm, Rebel, chrisw
-
- Posts: 4052
- Joined: Thu May 15, 2008 9:57 pm
- Location: Berlin, Germany
- Full name: Sven Schüle
Re: Simplifying code
In your last code snippet the second condition "(i >= prioQ.Count())" is always false due to the previous one ...
What was two times slower, the whole search?
What was two times slower, the whole search?
Sven Schüle (engine author: Jumbo, KnockOut, Surprise)
-
- Posts: 7220
- Joined: Mon May 27, 2013 10:31 am
Re: Simplifying code
Yes whole search is two times slower to something that was incorrect. So comparison does not make sense.
Pity I thought speed was back to 600-1000kn/second. But It is hardly 300kn/second.
By the way making ChessPosition a struct did not work too for linq was complaining about that properties were not accessable
for a struct using that linq construction.
Only works if I make method static and pass position as an argument. But that means it copies a position. So not efficient.
By the way I have more methods with position as an argument. So that would mean many unnecessary copies.
So ChessPosition stays a class so passing it will be by reference.
I made ChessBoardBits a struct. But no performance gain as expected. But I did not check any further. Might be there are methods with ChessBoardBits as a parameter giving unnecessary copies slowing down performance.
Code above does not compile if ChessPosition is a struct.
Using your code for NextMove
Only difference i before stage in return value.
Pity I thought speed was back to 600-1000kn/second. But It is hardly 300kn/second.
By the way making ChessPosition a struct did not work too for linq was complaining about that properties were not accessable
for a struct using that linq construction.
Only works if I make method static and pass position as an argument. But that means it copies a position. So not efficient.
By the way I have more methods with position as an argument. So that would mean many unnecessary copies.
So ChessPosition stays a class so passing it will be by reference.
I made ChessBoardBits a struct. But no performance gain as expected. But I did not check any further. Might be there are methods with ChessBoardBits as a parameter giving unnecessary copies slowing down performance.
Code: Select all
public void CollectCaptures(Builder builder, ulong source, ulong target,
bool excludePromotions)
{
var iter = new BitBoardIterator(0);
builder.AddRange(
new BitBoardIterator(source).Select(b =>
{
var sq = BoardSquares[b];
var sort = Board.PieceSort(sq);
iter.Init(sq.Moves(sort, Board.Occupiers) & target);
return CollectCaptures(iter, sq.MovesDict[(int)sort], excludePromotions);
}
).Aggregate((x, y) => x.AddRange(y))
);
}
Using your code for NextMove
Code: Select all
(IMovePrioQ, IMoveBase, int, int) NextMove(SearchParam p, IMovePrioQ prioQ, int stage, int i, bool allowStandPat)
{
if (stage > 3)
{
return (prioQ, null, -1, -1);
}
if (i >= prioQ.Count())
{
if (stage == 3) return (prioQ, null, -1, -1);
return NextMove(p, CollectPseudoLegalMoves(p, stage + 1, allowStandPat), stage + 1, 0, allowStandPat);
}
var mv = prioQ[i];
if (stage > 1 && !LegalMove(p.inCheck, mv, p.position)
|| stage == 3 && allowStandPat && !p.position.GivesCheck(mv))
{
return NextMove(p, prioQ, stage, i + 1, allowStandPat);
}
else
return (prioQ, mv, i, stage);
}
-
- Posts: 4052
- Joined: Thu May 15, 2008 9:57 pm
- Location: Berlin, Germany
- Full name: Sven Schüle
Re: Simplifying code
Which ! was missing? The one in this snippet above has always been there since you posted your NextMove() code. Omitting it would mean (if I understand your code correctly) that your qsearch would include all non-checking quiet moves if allowStandPat == true, which would effectively search one ply deeper than desired (but less accurate due to omitting quiet checks).
Sven Schüle (engine author: Jumbo, KnockOut, Surprise)
-
- Posts: 7220
- Joined: Mon May 27, 2013 10:31 am
Re: Simplifying code
It is about this code. Don't ask me what it is doing. I think it is crap. At least I don't understand it.
By the way I also restore ChessBoardBits. Making it a class again. For I like to control when object is copied and in case of struct it is always a copy. Or I should use ''ref' in C#. But the compiler complains that ref can not be used for a property and ChessBoardBits is a property of ChessPosition.
Code: Select all
bool? noMoreMovesInStandPat = null;
if (!noMove)
{
noMoreMovesInStandPat = stage == 3 && allowStandPat && p.position.GivesCheck(mv);
noMove = noMoreMovesInStandPat.Value;
}
if (noMove)
{
if (noMoreMovesInStandPat.HasValue && noMoreMovesInStandPat.Value)
return (prioQ, null, -1, -1);
else
return NextMove(p, prioQ, stage, i + 1, allowStandPat);
}
else
return (prioQ, mv, i, stage);
-
- Posts: 4052
- Joined: Thu May 15, 2008 9:57 pm
- Location: Berlin, Germany
- Full name: Sven Schüle
Re: Simplifying code
I don't ask ... but I agree that it looks suspicious. The meaning of "noMoreMovesInStandPat" is unclear and its implementation looks wrong since it appears to cut off all possibly remaining quiet checks during qsearch based on a property of just the current move (which is a check or not, depending on whether you forget the "!" before "p.position.GivesCheck(mv)" or not). So regardless whether the "!" is present, I think the whole version of NextMove() that uses this "noMoreMovesInStandPat" variable was useless, so it seems your decision to revert it should be fine.
Now if you compare search speed with NextMove() in this "old" version which you initially posted:
to your current version:
Is there any substantial difference in overall search speed? And do both work correctly (or at least, functionally identical)?
Now if you compare search speed with NextMove() in this "old" version which you initially posted:
Code: Select all
(IMovePrioQ, IMoveBase, int, int) NextMove( SearchParam p, IMovePrioQ prioQ, int stage, int i, bool allowStandPat)
{
var end = i >= prioQ.Count();
var (j, nextStage) = end ? (0, stage+1) : (i, stage);
var nextPrioQ = (end && (nextStage == 2 || nextStage == 3)) ? new MovePrioQ(
(nextStage == 2 ? p.position.CollectCapturesPromotions(true): p.position.CollectNonCaptures()
).Select(m =>
(ComputePriority(PSQTableEval, allowStandPat, KillerMoves,
MoveRatings, m, p.position, p.depth), m)).ToImmutableList()).Sort()
: prioQ;
var mv = j < nextPrioQ.Count() ? nextPrioQ[j]: null;
return (mv == null
|| nextStage > 1 && !LegalMove(p.inCheck, mv, p.position)
|| nextStage == 3 && allowStandPat && !p.position.GivesCheck(mv)
|| nextStage > 3
)
? (nextStage > 3) ? (nextPrioQ, null, -1, -1): NextMove(p, nextPrioQ, nextStage, j + 1, allowStandPat)
: (nextPrioQ, mv, j, nextStage);
}
Code: Select all
(IMovePrioQ, IMoveBase, int, int) NextMove(SearchParam p, IMovePrioQ prioQ, int stage, int i, bool allowStandPat)
{
if (stage > 3)
{
return (prioQ, null, -1, -1);
}
if (i >= prioQ.Count())
{
if (stage == 3) return (prioQ, null, -1, -1);
return NextMove(p, CollectPseudoLegalMoves(p, stage + 1, allowStandPat), stage + 1, 0, allowStandPat);
}
var mv = prioQ[i];
if (stage > 1 && !LegalMove(p.inCheck, mv, p.position)
|| stage == 3 && allowStandPat && !p.position.GivesCheck(mv))
{
return NextMove(p, prioQ, stage, i + 1, allowStandPat);
}
else
return (prioQ, mv, i, stage);
}
Sven Schüle (engine author: Jumbo, KnockOut, Surprise)
-
- Posts: 7220
- Joined: Mon May 27, 2013 10:31 am
Re: Simplifying code
I have to test later. System not alive. For in the meantime I started rewriting ChessBoardBits. But is not working yet. Being obsessed by using immutable datatypes. So I started to make array of bitboards immutable.
Think both versions posted are equally fast/slow because generating pseudo legal moves is slowest operation. Second place is test whether move is legal.
Current version of eval is doing not much more than returning piece square value.
[Also have a plan to add an extra stage for collecting captures of heavy pieces.
For if a pawn captures a rook there is a good chance no extra moves need to be generated]
Think both versions posted are equally fast/slow because generating pseudo legal moves is slowest operation. Second place is test whether move is legal.
Current version of eval is doing not much more than returning piece square value.
[Also have a plan to add an extra stage for collecting captures of heavy pieces.
For if a pawn captures a rook there is a good chance no extra moves need to be generated]
-
- Posts: 7220
- Joined: Mon May 27, 2013 10:31 am
Re: Simplifying code
Ok results from initial position. First one is yours. Second one was my original one.
So about same speed for speed of my pc is not constant.
So about same speed for speed of my pc is not constant.
Code: Select all
1 27 6 2 b1c3
2 19 8 45 b1c3 b8c6
3 27 9 213 b1c3 b8c6 g1f3
4 19 11 2186 b1c3 b8c6 g1f3 g8f6
5 -20 14 5220 b1c3 b8c6 g1f3 g8f6 a1b1
6 19 27 17299 b1c3 b8c6 g1f3 g8f6 a1b1 a7a5
7 19 53 42266 b1c3 b8c6 g1f3 g8f6 e2e4 a7a5 f1d3
8 19 130 131742 b1c3 b8c6 g1f3 g8f6 e2e4 e7e5 f1d3 f8d6
9 15 431 531100 b1c3 b8c6 d2d4 g8f6 c1f4 f6h5 f4e3 h5f6 g1f3
10 15 1275 1581668 b1c3 b8c6 d2d4 g8f6 c1f4 e7e6 g1f3 f8e7 a1b1 e8g8
11 13 2993 4210710 b1c3 b8c6 d2d4 g8f6 d4d5 c6e5 g1f3 e5f3 e2f3 e7e6 d5e6 f7e6
1 27 7 2 b1c3
2 19 10 45 b1c3 b8c6
3 27 11 213 b1c3 b8c6 g1f3
4 19 14 2186 b1c3 b8c6 g1f3 g8f6
5 -20 18 5220 b1c3 b8c6 g1f3 g8f6 a1b1
6 19 33 17299 b1c3 b8c6 g1f3 g8f6 a1b1 a7a5
7 19 60 42266 b1c3 b8c6 g1f3 g8f6 e2e4 a7a5 f1d3
8 19 132 131742 b1c3 b8c6 g1f3 g8f6 e2e4 e7e5 f1d3 f8d6
9 15 434 531100 b1c3 b8c6 d2d4 g8f6 c1f4 f6h5 f4e3 h5f6 g1f3
10 15 1292 1581668 b1c3 b8c6 d2d4 g8f6 c1f4 e7e6 g1f3 f8e7 a1b1 e8g8
11 13 3138 4210710 b1c3 b8c6 d2d4 g8f6 d4d5 c6e5 g1f3 e5f3 e2f3 e7e6 d5e6 f7e6
-
- Posts: 7220
- Joined: Mon May 27, 2013 10:31 am
Re: Simplifying code
Don't know. I get the impression that if you adopt functional programming style [I mean writing code that doesn't update variables] then you have to create a function for almost each loop or if statement.
Otherwise you may end up writing complicated nested expressions that are difficult to maintain or worse no one understands.
Otherwise you may end up writing complicated nested expressions that are difficult to maintain or worse no one understands.
-
- Posts: 7220
- Joined: Mon May 27, 2013 10:31 am
Re: Simplifying code
First I had this. But I don't want to create a class for each method that updates the state (say TranspositionTable, NodeCount, KillerMoves etc)
So second try:
Because function below has eight parameters.
But then you get a call like this.
Really simplfying (not).
Code: Select all
public interface IStateFunc<B, S>
{
(B res, S state) Apply();
}
class ABState<S>
{
public S State
{
get;
private set;
}
public ABState(S state)
{
State = state;
}
public B Update<B>(IStateFunc<B, S> f)
{
var (res, state) = f.Apply();
State = state;
return res;
}
}
Code: Select all
class ABState2<S>
{
public S State
{
get;
private set;
}
public ABState2(S state)
{
State = state;
}
public B Update<A1, A2, A3, A4, A5, A6, A7, A8,B>
(Func<A1, A2, A3, A4, A5, A6, A7, A8, (B, S) > f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5, A6 a6, A7 a7, A8 a8)
{
var (res, state) = f(a1, a2, a3, a4, a5, a6, a7, a8);
State = state;
return res;
}
}
Code: Select all
Func<ABSearch, int, int, IMoveBase, SearchParam,bool, double, StateContent, ((int?, ILine), StateContent)> CompMvValue
= delegate
(ABSearch search, int mvCount, int lb, IMoveBase mv,SearchParam p, bool check, double nextPsqValue, StateContent stateContent)
{
var nextPosHistory = search.UpdateHistory(p, p.positionHistory);
var nextPos = p.position.Move(mv);
if (p.depth <= 0
|| mvCount <= 0
|| (p.inCheck || check)
|| mv.GetCaptureLocation(p.position) != null
)
{
var (result, nextNodeCount, nextTTable) = search.DoABSearch(new SearchParam(stateContent.NodeCount + 1, false, nextPsqValue, nextPos,
nextPosHistory, null,
p.depth - R, p.plyCount + 1, -p.ub, -lb, check), stateContent.TTable);
return !result.value.HasValue ? ((null, null), new StateContent(nextNodeCount, nextTTable))
: ((-result.value, result.bestLine),
new StateContent(nextNodeCount, nextTTable));
}
else
{
int reduction = Min(p.depth, p.depth > 2 * R ? R + mvCount * p.depth / 50 : R);
var (result, nextNodeCount, nextTTable) = search.DoABSearch(new SearchParam(stateContent.NodeCount + 1, false, nextPsqValue, nextPos, nextPosHistory, null,
p.depth - reduction, p.plyCount + 1, -(lb + 1), -lb, check), stateContent.TTable);
if (!result.value.HasValue)
{
return ((null, null), new StateContent(nextNodeCount, nextTTable));
}
var mvVal = -result.value;
if (mvVal > lb && (lb != p.ub - 1 || reduction > R))
{
var (result2, nextNodeCount2, nextTTable2) = search.DoABSearch(new SearchParam(nextNodeCount, false, nextPsqValue, nextPos, nextPosHistory, null,
p.depth - R, p.plyCount + 1, -p.ub, -lb, check), nextTTable);
var researchValue = !result2.value.HasValue ? null : -result2.value;
return ((researchValue, result2.bestLine), new StateContent(nextNodeCount2, nextTTable2));
}
else
{
return ((mvVal, result.bestLine), new StateContent(nextNodeCount, nextTTable));
}
}
};
But then you get a call like this.
Code: Select all
...
var abState2 = new ABState2<StateContent>(stateContent);
var (mvValue, counterLine) = abState2.Update
<ABSearch, int, int, IMoveBase, SearchParam,bool, double,
StateContent, (int?, ILine)>
(CompMvValue, this, mvCount, lb, mv, p, check, nextPsqValue, stateContent);
...
Really simplfying (not).