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

Reduce big time spikes by reducing PV re-searches above original depth. #3192

Closed
wants to merge 1 commit into from

Conversation

xoto10
Copy link
Contributor

@xoto10 xoto10 commented Oct 21, 2020

Save time by reducing PV re-searches above original depth. Instead use 5% extra time on every move.

STC 10+0.1 th 1 :
LLR: 2.93 (-2.94,2.94) {-0.25,1.25}
Total: 90688 W: 9702 L: 9436 D: 71550
Ptnml(0-2): 408, 7252, 29792, 7450, 442
https://tests.stockfishchess.org/tests/view/5f8df807bacb75a4f9a47223

LTC 60+0.6 th 1 :
LLR: 2.97 (-2.94,2.94) {0.25,1.25}
Total: 97856 W: 4602 L: 4303 D: 88951
Ptnml(0-2): 53, 3757, 41057, 3960, 101
https://tests.stockfishchess.org/tests/view/5f8ec4872c92c7fe3a8c602d

Bench 3943959

…e 5% extra time on every move.

Bench 3943959
@vondele
Copy link
Member

vondele commented Oct 21, 2020

question on the implementation. Can't you use a local variable maxNextDepth computed as depth + 1 on function entry, instead of adding this as an argument to search?

@xoto10
Copy link
Contributor Author

xoto10 commented Oct 21, 2020

I'm not sure. My idea was that the top level call uses (... adjustedDepth, adjustedDepth ...), and the PV call uses min(maxNextDepth, newDepth) so preventing the PV re-search from being deeper than the original iteration depth. This suggests puttng adjustedDepth into the thread structure to me instead of using the extra fn parameter, and using min(thisThread->adjustedDepth, newDepth) in the PV call, but I think I tried that and it didn't work. Perhaps I did it wrong / differently somehow?

@vondele
Copy link
Member

vondele commented Oct 21, 2020

I see the difference between the call at the root and the others now. I need a closer look first.. (something like rootNode ? ... : ... + 1), which will have to wait a bit.

@xoto10
Copy link
Contributor Author

xoto10 commented Oct 21, 2020

Perhaps I had the right idea with my last test on the idpv2 branch, but 10% extra time was too much? Might be worth trying that with only 5% extra time.

@vondele
Copy link
Member

vondele commented Oct 21, 2020

@xoto10 vondele@cc7e03d is the variant I had in mind. Gives the same bench. What do you think?

@xoto10
Copy link
Contributor Author

xoto10 commented Oct 21, 2020

@vondele
Yeah, that makes sense. When I thought about it earlier on, I didn't think it would work for some reason, but now I look at it again it looks good. You could just test rootNode in the min() and then it's just a 1 line change (plus the time change). Nice :)

You've probably seen my test with adjustedDepth in the thread structure. Seems pointless now. If you start a test for yours I'll stop mine ...

@vondele
Copy link
Member

vondele commented Oct 21, 2020

since my variant is really equivalent to this PR (same bench), it needs no test IMO.

@xoto10
Copy link
Contributor Author

xoto10 commented Oct 21, 2020

fair enough!

@xoto10
Copy link
Contributor Author

xoto10 commented Oct 21, 2020

How do you want to progress, update this pr or will you create a new one?

@vondele
Copy link
Member

vondele commented Oct 21, 2020

I'll merge & close this PR using my cleanup. No need for a new PR, just leave this open.

@vondele vondele added the to be merged Will be merged shortly label Oct 22, 2020
@vondele vondele closed this in f5dfad5 Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants