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

Use less time on recaptures #5189

Closed
wants to merge 2 commits into from
Closed

Conversation

xoto10
Copy link
Contributor

@xoto10 xoto10 commented Apr 24, 2024

Credit for the idea goes to peregrine in Discord.

Passed STC 10+0.1:
LLR: 2.95 (-2.94,2.94) <0.00,2.00>
Total: 75712 W: 19793 L: 19423 D: 36496
Ptnml(0-2): 258, 8487, 20023, 8803, 285
https://tests.stockfishchess.org/tests/view/662652623fe04ce4cefc48cf

Passed LTC 60+0.6:
LLR: 2.94 (-2.94,2.94) <0.50,2.50>
Total: 49788 W: 12743 L: 12404 D: 24641
Ptnml(0-2): 29, 5141, 14215, 5480, 29
https://tests.stockfishchess.org/tests/view/6627495e3fe04ce4cefc59b6

The code was updated slightly and tested for non-regression against the original code at STC:
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 41952 W: 10912 L: 10698 D: 20342
Ptnml(0-2): 133, 4825, 10835, 5061, 122
https://tests.stockfishchess.org/tests/view/662d84f56115ff6764c7e438

Bench 1479416

@peregrineshahin
Copy link
Contributor

peregrineshahin commented Apr 24, 2024

running this https://tests.stockfishchess.org/tests/view/6628859d3fe04ce4cefc6f37 with only the base change to verify it's not the responsible for the gain
Edit: paused STC
here is LTC https://tests.stockfishchess.org/tests/view/66288f5a3fe04ce4cefc6fd1

@xoto10
Copy link
Contributor Author

xoto10 commented Apr 24, 2024

Yeah, less time seems to be helping stc, no doubt about that from the tests I've been doing. Whether it helps ltc is the real question ...
This was why I tried the timetime4 tests - trying to get stc and ltc about right

src/engine.h Outdated Show resolved Hide resolved
Copy link
Member

@Disservin Disservin left a comment

Choose a reason for hiding this comment

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

uci is the wrong place to implement it, since it's supposed to be a small wrapper around the engine and this leaks internals to uci.

@xoto10
Copy link
Contributor Author

xoto10 commented Apr 25, 2024

this leaks internals to uci.

If we get a fen with moves via UCI, and the last move is a capture, I want to store the capture square and then pass it into search - UCI seemed the right place to do this to me and I don't see how else I could do it??
The moves are parsed and applied in Engine::set_position(), so I need to derive the square in there, and pass it back out.

Happy to change it if there's a better way ofc.

@Disservin
Copy link
Member

The point is that we are slowly making it possible to use stockfish as a library, in which case the dev (the library user) should only include the engine code and use the engine functions to get the desired behaviour, so implementing uci would be left up to him. This patch would require him to implement the logic of the capSq manually and then set it in the limits struct, which is also perhaps the wrong data structure for this as the comment states LimitsType struct stores information sent by GUI.

Another solution would be to save the previous capture square in the StateInfo, but this perhaps a bit overkill, you could also add a new data field to the SharedState and pass the data like that to search

@dubslow
Copy link
Contributor

dubslow commented Apr 27, 2024

the simple LTC has proven to be considerably less good (but it was good to have tested it ofc), so i vote we stop that second LTC and approve this functionality to be merged (once the abstraction parts are figured out ofc)

@Disservin
Copy link
Member

@xoto10 do you need some other input from me? or do you want me to take a look how we can implement this differently?

@xoto10
Copy link
Contributor Author

xoto10 commented Apr 27, 2024

@Disservin
I've coded recap2b, see requested test https://tests.stockfishchess.org/tests/view/662d84f56115ff6764c7e438
If you want me to revise that further I can of course.

The huge influx of cores to fishtest caught me by surprise (very welcome of course!).

@xoto10
Copy link
Contributor Author

xoto10 commented Apr 27, 2024

I like the limits struct, I was wondering if we could rename it, maybe Analysis,Request, AnalysisRequest, something like that and it could hold the position requested, my capSq and any other inputs from the controlling program, as well as the go command params.
Maybe make it part of Engine so that it exists as soon as the Engine is defined.

@xoto10
Copy link
Contributor Author

xoto10 commented Apr 27, 2024

The recap2b non-regression passed (much quicker than I expected) so it looks like it's a valid rework of the recap2 code.
https://tests.stockfishchess.org/tests/view/662d84f56115ff6764c7e438

With hindsight I don't think I should have adjusted the EvalLevel constants so that the recapture multiplier was above/below 1.0 but it's done now so I'll leave it.

Let me know if I need to modify the recap2b code further ...

src/search.cpp Outdated
@@ -446,9 +446,11 @@ void Search::Worker::iterative_deepening() {
double reduction = (1.48 + mainThread->previousTimeReduction) / (2.17 * timeReduction);
double bestMoveInstability = 1 + 1.88 * totBestMoveChanges / threads.size();
int el = std::clamp((bestValue + 750) / 150, 0, 9);
double recapture =
limits.capSq != SQ_NONE && limits.capSq == rootMoves[0].pv[0].to_sq() ? 0.955 : 1.005;
Copy link
Member

@Disservin Disservin Apr 28, 2024

Choose a reason for hiding this comment

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

dont think we need the limits.capSq != SQ_NONE check, right? It is already implied by limits.capSq == rootMoves[0].pv[0].to_sq() and here we should always have a valid pv move.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good spot. Testing for the second element (ponder) makes sense, but I guess the first one has to be there!

Copy link
Member

@Disservin Disservin left a comment

Choose a reason for hiding this comment

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

i think this is fine for now, modifying the struct Search::LimitsType from the go function is perhaps a bit weird but that part will likely be changed in the future

@Disservin Disservin added the to be merged Will be merged shortly label Apr 28, 2024
@Disservin Disservin closed this in 886ed90 Apr 28, 2024
peregrineshahin added a commit to peregrineshahin/Stockfish that referenced this pull request Apr 28, 2024
commit 0fe6428
Author: Stefan Geschwentner <stgeschwentner@gmail.com>
Date:   Sun Apr 28 16:53:47 2024 +0200

    More reduction at cut nodes which are not a former PV node

    But the tt move and first killer are excluded.

    This idea is based on following LMR condition tuning
    https://tests.stockfishchess.org/tests/view/66228bed3fe04ce4cefc0c71 by
    using only the two largest terms P[0] and P[1].

    Passed STC:
    LLR: 2.93 (-2.94,2.94) <0.00,2.00>
    Total: 173248 W: 45091 L: 44565 D: 83592
    Ptnml(0-2): 693, 20534, 43673, 21002, 722
    https://tests.stockfishchess.org/tests/view/6629603b3fe04ce4cefc7d37

    Passed LTC:
    LLR: 2.94 (-2.94,2.94) <0.50,2.50>
    Total: 722394 W: 183231 L: 181487 D: 357676
    Ptnml(0-2): 462, 80650, 197252, 82348, 485
    https://tests.stockfishchess.org/tests/view/662cbe45d46f72253dcff7bf

    closes official-stockfish#5199

    Bench: 1619613

commit 48a3b7c
Author: Stefan Geschwentner <stgeschwentner@gmail.com>
Date:   Sun Apr 28 16:04:28 2024 +0200

    Simplify non-pawn material divisor to a constant

    Passed STC:
    https://tests.stockfishchess.org/tests/view/662942603fe04ce4cefc7aba
    LLR: 2.93 (-2.94,2.94) <-1.75,0.25>
    Total: 272832 W: 70456 L: 70497 D: 131879
    Ptnml(0-2): 1020, 32619, 69154, 32628, 995

    Passed LTC:
    https://tests.stockfishchess.org/tests/view/662dfe3b6115ff6764c829eb
    LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
    Total: 100254 W: 25446 L: 25303 D: 49505
    Ptnml(0-2): 121, 11292, 27166, 11419, 129

    closes official-stockfish#5198

    Bench: 1544645

commit 834e8ff
Author: cj5716 <125858804+cj5716@users.noreply.github.com>
Date:   Sun Apr 28 08:53:28 2024 +0800

    Penalise the TT move in multicut

    Passed STC:
    LLR: 2.99 (-2.94,2.94) <0.00,2.00>
    Total: 185504 W: 48079 L: 47533 D: 89892
    Ptnml(0-2): 716, 21866, 46988, 22520, 662
    https://tests.stockfishchess.org/tests/view/662d9e1d6115ff6764c7f83d

    Passed LTC:
    LLR: 2.94 (-2.94,2.94) <0.50,2.50>
    Total: 75612 W: 19351 L: 18948 D: 37313
    Ptnml(0-2): 46, 8363, 20592, 8752, 53
    https://tests.stockfishchess.org/tests/view/662dc9dc6115ff6764c80fea

    closes official-stockfish#5195

    Bench: 1415435

commit a129c06
Author: mstembera <m_stembera@yahoo.com>
Date:   Sun Apr 28 10:28:25 2024 -0700

    Combine remove and add in update_accumulator_refresh_cache()

    Combine remove and add in update_accumulator_refresh_cache().
    Move remove before add to match other parts of the code.

    STC:
    https://tests.stockfishchess.org/tests/view/662d96dc6115ff6764c7f4ca
    LLR: 2.95 (-2.94,2.94) <0.00,2.00>
    Total: 364032 W: 94421 L: 93624 D: 175987
    Ptnml(0-2): 1261, 41983, 94811, 42620, 1341

    closes official-stockfish#5194

    Bench: 1836777

commit 940a3a7
Author: mstembera <m_stembera@yahoo.com>
Date:   Thu Apr 25 18:20:08 2024 -0700

    Cache small net w/ psqtOnly support

    Caching the small net in the same way as the big net allows them to
    share the same code path and completely removes
    update_accumulator_refresh().

    STC:
    https://tests.stockfishchess.org/tests/view/662bfb5ed46f72253dcfed85
    LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
    Total: 151712 W: 39252 L: 39158 D: 73302
    Ptnml(0-2): 565, 17474, 39683, 17570, 564

    closes official-stockfish#5194

    Bench: 1836777

commit bc45cbc
Author: Joost VandeVondele <Joost.VandeVondele@gmail.com>
Date:   Sat Apr 27 18:09:45 2024 +0200

    Output some basic info about the used networks

    Adds size in memory as well as layer sizes as in

    info string NNUE evaluation using nn-ae6a388e4a1a.nnue (132MiB, (22528, 3072, 15, 32, 1))
    info string NNUE evaluation using nn-baff1ede1f90.nnue (6MiB, (22528, 128, 15, 32, 1))

    For example, the size in MiB is useful to keep the fishtest memory sizes up-to-date,
    the L1-L3 sizes give a useful hint about the architecture used.

    closes official-stockfish#5193

    No functional change

commit 3502c8a
Author: Disservin <disservin.social@gmail.com>
Date:   Thu Apr 25 19:20:57 2024 +0200

    Fix missing initialization of AccumulatorCaches in Eval::trace

    Add a constructor to `AccumulatorCaches` instead of just calling
    `clear(networks)` to prevent similar issues from appearing in the
    future.

    fixes official-stockfish#5190

    closes official-stockfish#5191

    No functional change

commit 886ed90
Author: xoto10 <23479932+xoto10@users.noreply.github.com>
Date:   Sun Apr 28 16:27:40 2024 +0100

    Use less time on recaptures

    Credit for the idea goes to peregrine on discord.

    Passed STC 10+0.1:
    https://tests.stockfishchess.org/tests/view/662652623fe04ce4cefc48cf
    LLR: 2.95 (-2.94,2.94) <0.00,2.00>
    Total: 75712 W: 19793 L: 19423 D: 36496
    Ptnml(0-2): 258, 8487, 20023, 8803, 285

    Passed LTC 60+0.6:
    https://tests.stockfishchess.org/tests/view/6627495e3fe04ce4cefc59b6
    LLR: 2.94 (-2.94,2.94) <0.50,2.50>
    Total: 49788 W: 12743 L: 12404 D: 24641
    Ptnml(0-2): 29, 5141, 14215, 5480, 29

    The code was updated slightly and tested for non-regression against the
    original code at STC:

    LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
    Total: 41952 W: 10912 L: 10698 D: 20342
    Ptnml(0-2): 133, 4825, 10835, 5061, 122
    https://tests.stockfishchess.org/tests/view/662d84f56115ff6764c7e438

    closes official-stockfish#5189

    Bench: 1836777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants