Skip to content

Commit

Permalink
Fix issue 2019. Bench: 3275144
Browse files Browse the repository at this point in the history
  • Loading branch information
@vondele
vondele committed Feb 26, 2019
1 parent badb2ac commit 4c9a684
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion src/search.cpp
View file
Expand Up @@ -915,7 +915,8 @@ namespace {

// Skip quiet moves if movecount exceeds our FutilityMoveCount threshold
moveCountPruning = depth < 16 * ONE_PLY
&& moveCount >= FutilityMoveCounts[improving][depth / ONE_PLY];
&& moveCount >= FutilityMoveCounts[improving][depth / ONE_PLY]
&& bestValue > VALUE_MATED_IN_MAX_PLY;

// Step 13. Extensions (~70 Elo)

Expand Down

4 comments on commit 4c9a684

@joergoster
Copy link

Choose a reason for hiding this comment

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

@vondele Yes, this is a very tempting solution. But we already have this condition in step 14 of our main search. Why do we need the exact same condition once more?
Answer: because the committed patch broke the logic! It seems much more appropriate to simply revert the commit!

@vondele
Copy link
Owner Author

Choose a reason for hiding this comment

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

reverting is one option, but the fact that nobody (neither the author nor the reviewers) saw this issue (and it is not so easy to spot) also means the code is not obvious, nor clear. Basically, moveCountPruning should not be true if there is a mate threat.. this patch is one way how to do it. I'll also have a look at an alternative, but I think this patch is reasonable as well.

@joergoster
Copy link

Choose a reason for hiding this comment

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

@vondele This is not exactly true. Jarrod (@DU-jdto) already pointed to some possible issues in the original PR. I don't blame anyone for not spotting this corner-case, of course! But can we be sure there are no further possible issues, even very rare ones?
It also seems to be more logical conceptually, to have one boolean control the LMP stuff, and another one for the move ordering stuff. But maybe that's just me. ;-)

@vondele
Copy link
Owner Author

Choose a reason for hiding this comment

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

certainly no blame intended (on the contrary, clearly good sense by @DU-jdto of what could go wrong).

Anyway, we need to fix this issue, one way or another, so let's see what the options are. Note that skipQuiets is pruning, not move ordering.

Please sign in to comment.