Small cleanups 06 #2628

vondele · 2020-04-13T08:44:56Z

Starting with an empty PR to collect suggestions for small cleanups.

No functional change.

Starting with an empty PR to collect suggestions for small cleanups. No functional change.

protonspring · 2020-04-13T15:47:16Z

protonspring/Stockfish@de9fc53...2fd2477

Vizvezdenec · 2020-04-13T19:52:00Z

This is not big, but...
We have all search bools starting from lowercase letters.
cutNode, rootNode, captureOrPromotion, singularLMR, etc.
Only PvNode for some reason starts from capital letter which sometime causes me to white patch in a wrong way, then read compiler message (no CutNode bool, proposing cutNode bool), swear, rewrite...
I guess it will be logical to have all this bools starting from lowercase letters for consistency.

vondele · 2020-04-13T20:02:21Z

@Vizvezdenec the idea behind is that PvNode is a constexpr.... i.e. known at compile time. cutNode is known only at runtime hence small. Other example are RazorMargin (compile time constant), like most other compile time constants, but we're not 100% consistent e.g. ttHitAverageWindow (should be TtHitAverageWindow).

Vizvezdenec · 2020-04-13T20:04:47Z

I'm just saying that PvNode is the same caliber of bool as rootNode and cutNode - and they are both from lowercase.
They are often used in the same expressions or close to each other. This is kinda inconsistent imho :) Sure it's constexpr or w/e but it's mostly used with not constexpr statements anyway.
After all it's like
If it's [alpha; beta] node it's rootNode for ply 0, PvNode for ply >= 0, if it's > beta it's cutNode, this seems kinda strange from logical point of view :)

protonspring · 2020-04-13T23:27:57Z

These are faster on my machines, and, to me, they are easier to read/understand.

master...protonspring:ps_relatives3

Lolligerhans · 2020-04-15T00:54:15Z

I have this refactoring of shift<>(bb), removing the excessive repetition of left-/right shifts, directions and masking. I did not test the changes because the executable is identical on my setup.
https://github.com/official-stockfish/Stockfish/compare/master...Lolligerhans:shift?expand=1

E: link on top of this branch https://github.com/vondele/Stockfish/compare/smallCleanups06...Lolligerhans:shift?expand=1

If the change is welcomed I can add comments or change formatting.

Lolligerhans · 2020-04-16T02:09:40Z

Complexity can use clamp.
https://github.com/official-stockfish/Stockfish/compare/master...Lolligerhans:cleanups?expand=1

protonspring · 2020-04-16T21:58:02Z

For pos.castling_rights(Color c), we can use the correct operator in types.h.

master...protonspring:ps_castlingrights11

protonspring · 2020-04-16T23:06:30Z

We don't need to ~pos.checkers() every time. This is a little faster:

master...protonspring:ps_sliderattacks3

master nps: 1606695, patch nps: 1634694

mstembera · 2020-04-17T00:10:20Z

@protonspring I originally thought having the ~pos.checkers() outside of the loop would be more efficient as well but as @Rocky640 pointed out here(and I verified)
#2584 (comment)
the loop gets executed less than once per call on average. Therefore having it inside the loop is actually better. (I could not measure any speed difference either way.)

Lolligerhans · 2020-04-17T07:42:38Z

Personally, I had an easier time if there were bakward_* version of forward_ranks_bb and -_file_bb. I can create a diff if required.

sb362 · 2020-04-17T11:34:38Z

There's an unnecessary square_bb() in Position::set_castling_right() which can be removed
sb362@28a8abe

silversolver1 · 2020-04-18T20:42:18Z

Some very minor typos in comments in tbprobe.cpp: master...silversolver1:tbprobe_typos_1

mstembera · 2020-04-18T20:43:10Z

src/types.h

@@ -412,11 +412,11 @@ constexpr Rank rank_of(Square s) {
 }

 constexpr Square relative_square(Color c, Square s) {
-  return Square(s ^ (c * 56));
+  return c == WHITE ? s : flip_rank(s);


This and line 419 changes are actually somewhat slower on my machine. It seems that introducing branching is not a good idea.

I get bit by this occasionally. Patches test faster on my machine, but others say it's slower. Any ways to counteract that? How are you testing performance?

mstembera · 2020-04-18T22:29:14Z

@protonspring I use Fishbench from https://github.com/zardav/FishBench w/ 40 tests which eliminates/reduces variance from run to run and issues with turbo boost because both master and patch run simultaneously. It is known that sometimes different versions may perform slightly differently on different machines so getting others to verify speed differences is a good thing.

Rocky640 · 2020-04-19T21:22:10Z

@protonspring @mstembera
It would be interesting to know
a) which compiler is used
b) bmi or modern
c) window or linux

mstembera · 2020-04-20T00:39:45Z

@Rocky640 I used gcc 8.1, bmi2, & windows.

protonspring · 2020-04-20T00:49:30Z

@Rocky640 My numbers were on gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0. I don't remember enabling bmi, so it's probably not enabled.

vondele · 2020-04-20T05:14:34Z

@mstembera @protonspring next to the setup, the measured speedup/slowdown, and the estimated error for that would be useful as well.

mstembera · 2020-04-21T02:26:08Z

@vondele

Results for 40 tests for each version:
            Base      Test      Diff      
    Mean    1880457   1861728   18729     
    StDev   42071     32077     58858     

p-value: 0.375
speedup: -0.01

I personally subscribe to the guideline that when in doubt it's best to avoid branching because branches are usually slower and less deterministic in performance(due to branch prediction issues) than normal instructions. Master is also nicely symmetric for white and black.

linrock · 2020-04-24T17:38:40Z

some ideas:

add *.o object files to .gitignore in the repo root so you don't see a big list of untracked files when running git status
use a .gitattributes file in the repo root to collapse tuning branch core files in diffs by default to make SPSA run diffs easier to read (see github docs)
in source files, remove whitespace between comments and commented functions so that the comments show up as annotations for certain code editors

for example, with intellisense on in VSCode, when hovering over a function that's commented without whitespace between the comment and the function definition, you get a nice annotation.

some functions like attackedBy already don't have that whitespace:

but for functions where there's a space between the comment and the function, it's blank when there could be an annotation.

example: pseudo_legal currently has a whitespace between the comment and the definition, so hovering over this shows nothing:

when the whitespace is removed, hovering over this shows the comment, which is a lot more useful:

pb00068 · 2020-04-25T10:40:44Z

@vondele

search.cpp line 837

    improving =  (ss-2)->staticEval == VALUE_NONE ? (ss->staticEval > (ss-4)->staticEval
              || (ss-4)->staticEval == VALUE_NONE) : ss->staticEval > (ss-2)->staticEval;

can now be simplified into

    improving =  (ss-2)->inCheck ? (ss->staticEval > (ss-4)->staticEval
              || (ss-4)->inCheck) : ss->staticEval > (ss-2)->staticEval;

I verified it delivers same bench number (tested with default bench & stockfish.exe bench 16 1 21).
Please let me know if this needs testing on the framework.

vondele · 2020-04-25T10:44:34Z

@pb00068 no need to test.

vondele · 2020-04-28T22:23:12Z

@linrock concerning your suggestions.

white space between comments and functions... I'm in favor of picking some consistent scheme, right now we mix. If you have a diff I can apply to this branch that would be convenient.
.gitignore : not sure we should check in such a file, people might have local variants of the file to ignore files they have usually around (.epd, .out, .xy, etc..) and we don't want to collect those.
.gitattributes : to considered to produce cleaner diffs of SPSA runs. However, I think we should merge tune branch in master, it is core functionality.

Vizvezdenec · 2020-04-29T13:57:05Z

Why do we even need this variable?
https://github.com/official-stockfish/Stockfish/blob/master/src/search.cpp#L1027
https://github.com/official-stockfish/Stockfish/blob/master/src/search.h#L35
It's not used anywhere (AFAIK) apart from this part of code, it equals flat 0, it makes no sence from consistency point of view - all other variables in this part of code are just written as flat numbers and not as variables declared in search.h...
ss->staticEval + 235 + 172 * lmrDepth <= alpha
(*contHist[3])[movedPiece][to_sq(move)] < 27400
captureHistory[movedPiece][to_sq(move)][type_of(pos.piece_on(to_sq(move)))] < 0
!pos.see_ge(move, Value(-194) * depth
etc
And all attempts to change this parameter failed in recent years, and even if they didn't - you can change it right there and not dig into search.h to find out that it's indeed 0.

vondele · 2020-04-29T15:28:58Z

@Vizvezdenec this variable was historically request by zamar #1066 (comment) it makes sense since the same value (minus 1) must be used in Thread::clear otherwise the implementation is incorrect.

Small cleanups 06

fcde749

Starting with an empty PR to collect suggestions for small cleanups. No functional change.

Fixup: add comments in eval

43a8acb

vondele added 8 commits April 18, 2020 14:45

Merge branch 'master' into smallCleanups06

cc9bb74

Fixup: blockedCount

30861c4

Fixup: TtHitAverageResolution

33a81d9

Fixup: clamp complexity

c9da287

Fixup: use operator for castling rights

e00fc70

Fixup: remove unnecessary square_bb()

4869921

Fixup: relatives

bd7b990

Merge branch 'master' into smallCleanups06

8f657e3

mstembera reviewed Apr 18, 2020

View reviewed changes

vondele added 3 commits April 28, 2020 23:46

Fixup: add assert.

58a9f2e

Merge branch 'master' into smallCleanups06

cf9d5be

Fixup: improving..

a59bbb9

vondele closed this in 353e206 Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small cleanups 06 #2628

Small cleanups 06 #2628

vondele commented Apr 13, 2020

protonspring commented Apr 13, 2020

Vizvezdenec commented Apr 13, 2020

vondele commented Apr 13, 2020

Vizvezdenec commented Apr 13, 2020 •

edited

Loading

protonspring commented Apr 13, 2020

Lolligerhans commented Apr 15, 2020 •

edited

Loading

Lolligerhans commented Apr 16, 2020

protonspring commented Apr 16, 2020 •

edited

Loading

protonspring commented Apr 16, 2020 •

edited

Loading

mstembera commented Apr 17, 2020 •

edited

Loading

Lolligerhans commented Apr 17, 2020

sb362 commented Apr 17, 2020

silversolver1 commented Apr 18, 2020

mstembera Apr 18, 2020

protonspring Apr 18, 2020

mstembera commented Apr 18, 2020 •

edited

Loading

Rocky640 commented Apr 19, 2020

mstembera commented Apr 20, 2020

protonspring commented Apr 20, 2020

vondele commented Apr 20, 2020

mstembera commented Apr 21, 2020 •

edited

Loading

linrock commented Apr 24, 2020

pb00068 commented Apr 25, 2020

vondele commented Apr 25, 2020

vondele commented Apr 28, 2020

Vizvezdenec commented Apr 29, 2020 •

edited

Loading

vondele commented Apr 29, 2020

Small cleanups 06 #2628

Small cleanups 06 #2628

Conversation

vondele commented Apr 13, 2020

protonspring commented Apr 13, 2020

Vizvezdenec commented Apr 13, 2020

vondele commented Apr 13, 2020

Vizvezdenec commented Apr 13, 2020 • edited Loading

protonspring commented Apr 13, 2020

Lolligerhans commented Apr 15, 2020 • edited Loading

Lolligerhans commented Apr 16, 2020

protonspring commented Apr 16, 2020 • edited Loading

protonspring commented Apr 16, 2020 • edited Loading

mstembera commented Apr 17, 2020 • edited Loading

Lolligerhans commented Apr 17, 2020

sb362 commented Apr 17, 2020

silversolver1 commented Apr 18, 2020

mstembera Apr 18, 2020

Choose a reason for hiding this comment

protonspring Apr 18, 2020

Choose a reason for hiding this comment

mstembera commented Apr 18, 2020 • edited Loading

Rocky640 commented Apr 19, 2020

mstembera commented Apr 20, 2020

protonspring commented Apr 20, 2020

vondele commented Apr 20, 2020

mstembera commented Apr 21, 2020 • edited Loading

linrock commented Apr 24, 2020

pb00068 commented Apr 25, 2020

vondele commented Apr 25, 2020

vondele commented Apr 28, 2020

Vizvezdenec commented Apr 29, 2020 • edited Loading

vondele commented Apr 29, 2020

Vizvezdenec commented Apr 13, 2020 •

edited

Loading

Lolligerhans commented Apr 15, 2020 •

edited

Loading

protonspring commented Apr 16, 2020 •

edited

Loading

protonspring commented Apr 16, 2020 •

edited

Loading

mstembera commented Apr 17, 2020 •

edited

Loading

mstembera commented Apr 18, 2020 •

edited

Loading

mstembera commented Apr 21, 2020 •

edited

Loading

Vizvezdenec commented Apr 29, 2020 •

edited

Loading