Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speedup OppositBishop calculations #2592

Closed

Conversation

MJZ1977
Copy link
Contributor

@MJZ1977 MJZ1977 commented Mar 20, 2020

Save opposite bishops boolean in state info. Opposite bishops are called only if there is a bishop capture or a bishop promotion.

STC
http://tests.stockfishchess.org/tests/view/5e748183e42a5c3b3ca2e7de
LLR: 2.94 (-2.94,2.94) {-0.50,1.50}
Total: 88574 W: 17268 L: 16964 D: 54342
Ptnml(0-2): 1308, 9884, 21658, 10070, 1367

Bench 4952322

@snicolet
Copy link
Member

snicolet commented Mar 20, 2020

It depends on the appreciation of the maintainer.

Quoting from https://github.com/glinscott/fishtest/wiki/Creating-my-first-test#non-functional-changes :

We will verify the speed up, for 3 reasons:

  • The speedup needs to be statistically significant and not just random noise.
  • The speedup needs to be confirmed on different machines. Sometimes a speedup on one machine is a slowdown on another.
  • The speedup needs to be put in perspective with the code changes. If the code changes are invasive and/or ugly, only to achieve a small speedup, we will probably not accept the patch. This is subject to our appreciation.

If the above steps are not enough to clarify the opportunity to commit the patch, then the patch will go under normal STC+LTC fishtest tests. The rationale is that a speed-up is totally comparable to a normal patch: it adds complexity with the aim to improve ELO, so it makes sense to test under the same conditions.

@MJZ1977
Copy link
Contributor Author

MJZ1977 commented Mar 20, 2020

I think that there is some differences between a normal patch and a speedup:

  • speedup don't change algorithms, it just makes the calculations faster : almost no risk to make a regression in some special positions (but OK perhaps it used more hash in some cases so we need some tests)
  • the code is already optimized: it is not easy to make it much faster and pass LTC (it is like a feat nowadays).

@ddugovic
Copy link

Regarding this patch, this point is what I'm most curious about (in particular, the new member in position.h):

  • The speedup needs to be put in perspective with the code changes. If the code changes are invasive and/or ugly, only to achieve a small speedup, we will probably not accept the patch. This is subject to our appreciation.

@Vizvezdenec
Copy link
Contributor

I don't think that this code changes are ugly.
I mean, who the hell even watches how we canculate if we are in OCB position or not? This code wasn't touched for ages and wouldn't be touches for ages after it change.
I haven't looked there once as being one of the most active devs in recent times because I find it pretty useless :)

@ddugovic
Copy link

ddugovic commented Mar 21, 2020

This change increases the size of a Position object, so anyone whose computer has a finite amount of memory might care.

EDIT: I am mistaken; there is one object per thread so size of the object is less relevant.

@Sopel97
Copy link
Member

Sopel97 commented Mar 21, 2020

There is only one position per thread

@vondele
Copy link
Member

vondele commented Mar 23, 2020

This failed LTC testing : https://tests.stockfishchess.org/tests/view/5e74e4e6e42a5c3b3ca2e819
I'd like to take discussion of this to the open issue #2593

@vondele
Copy link
Member

vondele commented Mar 29, 2020

I'll close this after the discussion in issue #2593

@vondele vondele closed this Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants