[Code Analysis] Consider moving some data to heap

[calcitem]

Warning C6262 Function uses '40076' bytes of stack: exceeds /analyze:stacksize '16384'. Consider moving some data to heap.

misc.cpp

   // extract the working directory
    workingDirectory = "";
    char buff[40000];    // Warning C6262
    char* cwd = GETCWD(buff, 40000);
    if (cwd)
        workingDirectory = cwd;

[FireFather]

static char buff[40000]; should resolve it

[Sopel97]

This is not an issue. Just something MSVC (and only MSVC) considers unsafe. There's no need to do anything with it.

[FireFather]

Yes, it's indicated via MSVS Code Analysis... and something I've always addressed

[Sopel97]

We don't support MSVC so no one cares.

[FireFather]

Ok, if you do decide it to address it at some point, there are a couple more places where adding static storage would resolve the issue:

bitboard.cpp

149     static int seeds[][RANK_NB] = { { 8977, 44560, 54343, 38998,  5731, 95205, 104912, 17020 },
 150                                                         {  728, 10316, 55013, 32803, 12281, 15100,  16645,   255 } };
151
152     static Bitboard occupancy[4096], reference[4096], edges, b;
153     static int epoch[4096] = {}, cnt = 0, size = 0;

search.cpp

319     static Stack stack[MAX_PLY+10], *ss = stack+7;
230     static Move  pv[MAX_PLY+1];

[Sopel97]

search.cpp

319       static Stack stack[MAX_PLY+10], *ss = stack+7;
230       static Move  pv[MAX_PLY+1];

No, that would just introduce a bug.

[calcitem]

My question is, for such a large array, why not alloc space from the heap to improve applicability? The stack space is limited.

char buff[40000];
// Change to:
char *buff = new char(40000);

The use of new here will not cause a significant performance loss because it is not on the main path of the search algorithm.

Assuming it is performance sensitive, we can also improve performance by allocating a memory pool on the heap.

Reference: https://stackoverflow.com/questions/1825964/c-c-maximum-stack-size-of-program

[FireFather]

We don't support MSVC so no one cares.

That's quite unfortunate, because there are many who compile it with Visual Studio. Is that the official policy? Perhaps the README.md needs to be changed. "When not using the Makefile to compile (for instance, with Microsoft MSVC) you need to manually set/unset some switches in the compiler command line; see file types.h for a quick reference."

[FireFather]

search.cpp

319     static Stack stack[MAX_PLY+10], *ss = stack+7;
230     static Move  pv[MAX_PLY+1];

No, that would just introduce a bug.

I've tried it, it runs here just fine, and the stack overflow warnings in Code Analysis are gone. calcitem's above above of 'new' should also work.

[FireFather]

We don't support MSVC so no one cares.

MS Code Analysis is widely used and very reliable...thousands of major companies use it for their applications. According to NuGet, 101.6K downloads per day, 288.2M total https://www.nuget.org/packages/Microsoft.CodeAnalysis.Common/

Advice and indications from it are normally quite accurate.

[Sopel97]

search.cpp

319       static Stack stack[MAX_PLY+10], *ss = stack+7;
230       static Move  pv[MAX_PLY+1];

No, that would just introduce a bug.

I've tried it, it runs here just fine, and the stack overflow warnings in Code Analysis are gone. calcitem's above above of 'new' should also work.

try with setoption name Threads value 2 now

[Sopel97]

We don't support MSVC so no one cares.

MS Code Analysis is widely used and very reliable...thousands of major companies use it for their applications. According to NuGet, 101.6K downloads per day, 288.2M total https://www.nuget.org/packages/Microsoft.CodeAnalysis.Common/

Advice and indications from it are normally quite accurate.

I don't care who uses it. Static code analysis is only a suggestion. It is often wrong or opinionated.

[FireFather]

search.cpp

319     static Stack stack[MAX_PLY+10], *ss = stack+7;
230     static Move  pv[MAX_PLY+1];

No, that would just introduce a bug.

I've tried it, it runs here just fine, and the stack overflow warnings in Code Analysis are gone. calcitem's above above of 'new' should also work.

try with setoption name Threads value 2 now

Yes I see, it fails and I just noticed the comment: "To allow access to (ss-7) up to (ss+2), the stack must be oversized." Stack stack[MAX_PLY+10], *ss = stack+7; Move pv[MAX_PLY+1];

the code analysis runs on a single thread I believe I suppose it's the same situation for bitboard.cpp & misc.cpp..? as they emit the same warning concerning large arrays.

Thanks Sopel

[Sopel97]

The correct fix would be to use std::vector/std::unique_ptr. If it needed fixing.

[calcitem]

In performance-sensitive AI programs, in key algorithm modules, the use of STL needs to be very cautious. I implemented a stack class by myself in our project. It is very concise, without redundant memory allocation and exception checking, and it can also avoid some static analysis tools reporting errors.

https://github.com/calcitem/Sanmill/blob/dev/src/stack.h https://github.com/calcitem/Sanmill/blob/dev/src/search.cpp

[Sopel97]

In performance-sensitive AI programs, in key algorithm modules, the use of STL needs to be very cautious. I implemented a stack class by myself in our project. It is very concise, without redundant memory allocation and exception checking, and it can also avoid some static analysis tools reporting errors.

https://github.com/calcitem/Sanmill/blob/dev/src/stack.h

This only works with trivial types (but doesn't assert for it). It's definitely not safe. Either way, this is not a performance critical part.

[calcitem]

Thanks! BTW, Why I like to use VS is because it has a performance analyzer that is easier to use than gprof. At that time, many optimizations made in the search function of our program were the problems found by this performance analyzer. I believe there will be many people using it.

[FireFather]

For reference, here are the actual warnings from Code Analysis

C:\engine_dev\stockfish\search.cpp(282): warning C6262: Function uses '17560' bytes of stack:  exceeds /analyze:stacksize '16384'.  Consider moving some data to heap.
C:\engine_dev\stockfish\misc.cpp(629): warning C6262: Function uses '40068' bytes of stack:  exceeds /analyze:stacksize '16384'.  Consider moving some data to heap.
C:\engine_dev\stockfish\bitboard.cpp(146): warning C6262: Function uses '82056' bytes of stack:  exceeds /analyze:stacksize '16384'.  Consider moving some data to heap.

See my 3rd post above for the large arrays to which these warnings refer...

[vondele]

nothing to be done for this, we require quite some stack space, dynamic allocations are avoided for performance reasons.