[Merged] Smarter time management near stop limit

[xoto10]

This patch makes Stockfish search same depth again if > 60% of optimum time is
already used, instead of trying the next iteration. The idea is that the next
iteration will generally take about the same amount of time as has already been
used in total. When we are likely to begin the last iteration, as judged by total
time taken so far > 0.6 * optimum time, searching the last depth again instead of
increasing the depth still helps the other threads in lazy SMP and prepares better
move ordering for the next moves.

STC :
LLR: 2.95 (-2.94,2.94) {-1.00,3.00}
Total: 13436 W: 2695 L: 2558 D: 8183
Ptnml(0-2): 222, 1538, 3087, 1611, 253
https://tests.stockfishchess.org/tests/view/5e1618a761fe5f83a67dd964

LTC :
LLR: 2.94 (-2.94,2.94) {0.00,2.00}
Total: 32160 W: 4261 L: 4047 D: 23852
Ptnml(0-2): 211, 2988, 9448, 3135, 247
https://tests.stockfishchess.org/tests/view/5e162ca061fe5f83a67dd96d

The code was revised as suggested by @vondele for multithreading:

STC th 8 :
LLR: 2.95 (-2.94,2.94) {0.00,2.00}
Total: 16640 W: 2049 L: 1885 D: 12706
Ptnml(0-2): 119, 1369, 5158, 1557, 108
https://tests.stockfishchess.org/tests/view/5e19826a2cc590e03c3c2f52

LTC th 8 :
LLR: 2.95 (-2.94,2.94) {-1.00,3.00}
Total: 16536 W: 2758 L: 2629 D: 11149
Ptnml(0-2): 182, 1758, 4296, 1802, 224
https://tests.stockfishchess.org/tests/view/5e18b91a27dab692fcf9a140

Thanks to those discussing Stockfish lazy SMP on fishcooking which made me
try this, and to @vondele for suggestions and doing related tests.

See full discussion in the pull request thread:
#2482

Bench: 4586187

[xoto10]

I have modified the PR code to work on multiple threads since this code only uses mainthread, see master...xoto10:research3 I will start a test for this version.

Also of interest is the research1 branch, which failed STC but is possibly looking slightly better at 25+0.25. It's possible this code is scaling well, and it is a very simple change which scales to multiple threads without needing interaction between them.

Comments on how we should proceed with testing welcomed :-)

[Alayan-stk-2]

I view this PR as a time management tweak : TM is usually limited by the fact that searching another depth significantly increase the time spent on the move, so there are many situations where the choice is between underusing the expected time by not searching one depth more, or overusing it by doing it.

Searching the same depth again might not be optimal behavior on unlimited search (though research1 indicates that redoing the same depth from time to time is surprisingly competitive), but it clearly increases the average move strength compared to a single search (vondele's experiments on the topic are very interesting), while using less time than fully going to the next depth.

So in effect, it gives more granularity to how much time is used on the move, as if we could search a "half-ply depth" more.

[xoto10]

So in effect, it gives more granularity to how much time is used on the move, as if we could search a "half-ply depth" more.

Yes, this is what I think is happening. I haven't thought much about why the repeat search gains anything, but it clearly gains something, and is presumably quite fast, roughly half the time of searching the next depth and maybe half again (??) because most of the search should hit the TT. So the small gain in a quick time, plus not using much over the optimum time, combine to give a good result.

[xoto10]

The modified code for multithread passed STC (link added to PR text) so I have squashed and pushed that change.

[Alayan-stk-2]

I'd suggest not merging it for SF11, but rather have it as first patch of SF12-dev (or a variant of this patch depending on how experiments go), in order to have all the regression tests applicable to the actual SF11 release.

[xoto10]

rather have it as first patch of SF12-dev (or a variant of this patch depending on how experiments go)

Sure, whatever the maintainers decide. There are still some interesting tests going on, e.g. I'd like to see NMC_rootdepth1_4 continue but the ltc been put to prio -1 for some reason.

[ghost]

@xoto10 set to 0 priority, 50 throughput. I just run alot of tests right now, so it might seem to use disproportionately more resources.

[xoto10]

@Chess13234
Sure. It's good of you to be considerate to others, but that one is ltc and also +ve llr after 20k games :-)

[xoto10]

Went with vondele's suggested code and it has passed stc and ltc multithreaded tests nicely (test results added to PR text). I have rebased, updated, squashed and pushed that code.

vondele does have an interesting test running that is related to this PR. It has the advantage that the code is simpler and there is no interaction between threads, so it might be preferred over this change. Also it would work when a user is analysing a position using "go infinite", while this PR only works when thinking time is restricted.

[snicolet]

OK, so let's wait for Joost's LTC test to finish!

EDIT: Joost's running test
(https://tests.stockfishchess.org/tests/view/5e19704e2cc590e03c3c2f44) seems to be struggling, so it seems we will go with your version.

[snicolet]

I have seen concerns that some GUIs may have problems if Stockfish uses non-increasing depth search sequences, do you guys think there might be issues with this patch?

But I can't find the related discussions anymore..

[xoto10]

I have seen concerns that some GUIs may have problems if Stockfish uses non-increasing depth search sequences, do you guys think there might be issues with this patch?

@snicolet
See here and here
I ended up using @vondele's code exactly as suggested in the second link. Since this uses adjustedDepth to search again the rootDepth will keep increasing, this should avoid any problems with the GUI.

[vondele]

yes, this patch looks good to me. The GUI issue is taken care of as described. The interaction between threads, in this case seems good. It is, IMO, an outstanding issue that the time management is being controlled by master only. This patch kind of signals that master is about to stop the search, so threads get a kind of 'soft exit' signal (and research at the same depth).

[snicolet]

Merged via 69204f0, congrats :-)

[locutus2]

I see now after commit that in the commit message (and also the PR) accidently the SMP results for STC and LTC are exchanged.

[Alayan-stk-2]

@snicolet That's nice, but that invalidates the regression tests for SF11... Running them again (at least the regular one, I guess the scaling info from 10+0.1 and 180+1.8 that Viz ran don't need another) with so little patches in between is heavy on the framework.

[snicolet]

Honestly, I don't think that regression tests should be a reason for refusing to put in a nice Elo gaining idea for Stockfish 11 users. Progression tests results depends on the book used, on the time control, on number of threads, on playing against a previous version rather than a set of various opponents, etc.

Stockfish 11 will be promoted as being "50 Elo stronger than Stockfish 10", and this remains true after this patch :-)

[snicolet]

What are the implications of using ponder with this patch?

[vondele]

@snicolet right, that looks like a bug. Should it be ? :

          else if (   Threads.increaseDepth
                   && !mainThread->ponder
                   && Time.elapsed() > Time.optimum() * fallingEval * reduction * bestMoveInstability * 0.6)

[xoto10]

@snicolet Good spot!
@vondele That seems the easiest fix. Perhaps we can keep (some of?) the gains from this patch while pondering by making sure an iteration is only repeated once, but that would be a bit trickier to code and test ...

[vondele]

Right now I would not wonder about the details of pondering... we have no (fishtest) way to test....let's just get the behavior correct.

[xoto10]

I agree with your suggestion, can you create a PR to do that?

[vondele]

@xoto10 can you do that, I'll be busy today

FYI official-stockfish/fishtest#503

[xoto10]

Ok, will do. I'll look into the fishtest issue during the week as well ...

[Alayan-stk-2]

Honestly, I don't think that regression tests should be a reason for refusing to put in a nice Elo gaining idea for Stockfish 11 users. Progression tests results depends on the book used, on the time control, on number of threads, on playing against a previous version rather than a set of various opponents, etc.

Stockfish 11 will be promoted as being "50 Elo stronger than Stockfish 10", and this remains true after this patch :-)

Well, at this rate you could merge the two latest gainers, from SG and Viz, before actually doing the release commit.

Because SF11 will be the new base future progression tests will compare to, we'll have to run those again anyway.

[adentong]

This can be closed now, no?

[mstembera]

@xoto10 Do you know why increaseDepth can be set back to true once it has been set to false? See https://tests.stockfishchess.org/tests/view/63982862b4e52c95053f5cc7 Thanks

[xoto10]

I think the idea is that if the iteration altered the calculation of totalTime a lot: double totalTime = Time.optimum() * fallingEval * reduction * bestMoveInstability * complexPosition; then it might be appropriate to increase depth rather than keep it constant again.

…

On Tue, Dec 13, 2022 at 7:25 AM mstembera ***@***.***> wrote: @xoto10 <https://github.com/xoto10> Do you know why increaseDepth can be set back to true once it has been set to false? See https://tests.stockfishchess.org/tests/view/63982862b4e52c95053f5cc7 Thanks — Reply to this email directly, view it on GitHub <#2482 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFTEM7EIIBLMAU3MI2PAD7DWNAQGZANCNFSM4KEQOAUQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mstembera]

@xoto10 Thanks for explaining. I think what is intended then is https://tests.stockfishchess.org/tests/view/6398c74693ed41c57ede7bfd Does that make sense?

[xoto10]

So you're just removing the test for Threads.increaseDepth is that right? Yes, it seems redundant to me. Perhaps it saves one or two executions of Time.elapsed() but I can't see that being important at this point in the code. You could try a simplification to get rid of it and see what happens?

…

On Tue, Dec 13, 2022 at 6:42 PM mstembera ***@***.***> wrote: @xoto10 <https://github.com/xoto10> Thanks for explaining. I think what is intended then is https://tests.stockfishchess.org/tests/view/6398c74693ed41c57ede7bfd Does that make sense? — Reply to this email directly, view it on GitHub <#2482 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFTEM7BSS2V3X55ZACEVBZDWNC7RRANCNFSM4KEQOAUQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mstembera]

@xoto10 I think the test for Threads.increaseDepth is more than redundant. I makes it so that once it's set to false it will be turned back to true the very next iteration even if elapsed time at that point is say greater than 99% of total time. This seems like a bug to me. You are right it's also a simplification which i didn't consider. If the current test fails I will retest as such.

[xoto10]

Ah, yes.
I suggest you stop the ltc and run stc as a simplification. STC is cheap and should pass quickly as a simplification. If the current ltc fails you will need to try the simplification anyway, and if it passes, well, the simplification should pass even quicker.

[mstembera]

@xoto10 Ok LTC stopped. I don't think we need another STC simplification since it passed STC as an elo gain. New LTC here
https://tests.stockfishchess.org/tests/view/6399bcd393ed41c57edea750