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

Fix a data race for NNUE #2957

Closed
wants to merge 1 commit into from

Conversation

vondele
Copy link
Member

@vondele vondele commented Aug 9, 2020

the stateInfo at the rootPos is no longer read-only, as the NNUE accumulator is part of it.
Threads can thus not share this object, and need their own copy.

tested for no regression
https://tests.stockfishchess.org/tests/view/5f3022239081672066536bce
LLR: 2.96 (-2.94,2.94) {-1.50,0.50}
Total: 52800 W: 6843 L: 6802 D: 39155
Ptnml(0-2): 336, 4646, 16399, 4679, 340

fixes #2933

No functional change

@sf-x
Copy link

sf-x commented Aug 9, 2020

Sorry for my ignorance, but isn't it possible to simply call NNUE::evaluate on the root position? Once accumulator is updated, no further updates should be necessary.

@vondele
Copy link
Member Author

vondele commented Aug 9, 2020

no, I don't think so.

the stateInfo at the rootPos is no longer read-only, as the NNUE accumulator is part of it.
Threads can thus not share this object and need their own copy.

tested for no regression
https://tests.stockfishchess.org/tests/view/5f3022239081672066536bce
LLR: 2.96 (-2.94,2.94) {-1.50,0.50}
Total: 52800 W: 6843 L: 6802 D: 39155
Ptnml(0-2): 336, 4646, 16399, 4679, 340

fixes official-stockfish#2933

No functional change
@vondele vondele changed the title [WIP] Fix a data race for NNUE Fix a data race for NNUE Aug 9, 2020
@vondele vondele closed this in 27b593a Aug 9, 2020
@snits
Copy link

snits commented Aug 10, 2020

Using the development build of this at https://abrok.eu/stockfish/ (win x64 haswell) as soon as setoption name Use NNUE value true is sent, the engine seems to go bye bye. Edit: It looks like the thread value had no impact on whether it crashed.

@bknox83
Copy link
Contributor

bknox83 commented Aug 10, 2020

I'm seeing the same thing on the win x64 modern + AVX2 build.

@snits
Copy link

snits commented Aug 10, 2020

Trying from the command line I get to the point of telling it go infinite before it dies. I guess 'setoption name Use NNUE value true' is the last thing it was logging in the debug log when I ran it in chessbase. Also see it with the avx2 build like @bknox83. Testing further, I see the problem with the avoid special case commit build as well, but don't see it with the NNUE pawn commit, so it would appear perhaps the issue is the special case commit and not this.

@Coolchessguykevin
Copy link

Related issue is:

#2963

@TonHaver
Copy link

As part of this patch, a change to Makefile is made.
-mmacosx-version-min=10.14 (it was 10.13)

FYI: current master still compiles on MacOS 10.13 and a NNUE bench is given

@vondele
Copy link
Member Author

vondele commented Aug 10, 2020

@TonHaver this change was intentional, as the CI would fail with 10.13:

./thread.h:82:8: error: aligned deallocation function of type 'void (void *, std::align_val_t) noexcept' is only available on macOS 10.14 or newer

see https://travis-ci.org/github/official-stockfish/Stockfish/jobs/716330662

@TonHaver
Copy link

Ah, OK.
But then, don't we do as mentioned here:
./thread.h:82:8: note: if you supply your own aligned allocation functions, use -faligned-allocation to silence this diagnostic

Or is using mm_malloc and mm_free in this configuration something completely different?

@vondele
Copy link
Member Author

vondele commented Aug 10, 2020

We don't provide our own aligned allocation function for this object, AFAICT. This wouldpresumably be the path for a work-around to support 10.13, if somebody really sees an urgent need.

joergoster pushed a commit to joergoster/Stockfish-old that referenced this pull request Aug 10, 2020
the stateInfo at the rootPos is no longer read-only, as the NNUE accumulator is part of it.
Threads can thus not share this object and need their own copy.

tested for no regression
https://tests.stockfishchess.org/tests/view/5f3022239081672066536bce
LLR: 2.96 (-2.94,2.94) {-1.50,0.50}
Total: 52800 W: 6843 L: 6802 D: 39155
Ptnml(0-2): 336, 4646, 16399, 4679, 340

closes official-stockfish/Stockfish#2957

fixes official-stockfish/Stockfish#2933

No functional change
lucabrivio pushed a commit to lucabrivio/Stockfish that referenced this pull request Aug 10, 2020
the stateInfo at the rootPos is no longer read-only, as the NNUE accumulator is part of it.
Threads can thus not share this object and need their own copy.

tested for no regression
https://tests.stockfishchess.org/tests/view/5f3022239081672066536bce
LLR: 2.96 (-2.94,2.94) {-1.50,0.50}
Total: 52800 W: 6843 L: 6802 D: 39155
Ptnml(0-2): 336, 4646, 16399, 4679, 340

closes official-stockfish/Stockfish#2957

fixes official-stockfish/Stockfish#2933

No functional change
noobpwnftw pushed a commit to noobpwnftw/Stockfish that referenced this pull request Aug 15, 2020
the stateInfo at the rootPos is no longer read-only, as the NNUE accumulator is part of it.
Threads can thus not share this object and need their own copy.

tested for no regression
https://tests.stockfishchess.org/tests/view/5f3022239081672066536bce
LLR: 2.96 (-2.94,2.94) {-1.50,0.50}
Total: 52800 W: 6843 L: 6802 D: 39155
Ptnml(0-2): 336, 4646, 16399, 4679, 340

closes official-stockfish#2957

fixes official-stockfish#2933

No functional change
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.

Possible data race
6 participants