Intriguing! My immediate thought - will making those string buffers static break thread safety?

Hihi, using OpenTTD as a benchmark application .. things I never expected people to do :D Nice!

Question for you: I do not fully understand why that tool considers a stack variable "an allocation"; as that shouldn't be an allocation at all. At least, from what I see in the assembly code, these buffers don't end up on the heap, but stay on the stack. So the stack is just increased with, in this case, 2048, and the buffer sits there on the stack. That is not an "allocation" in the classical sense, as it doesn't happen on the heap.

So I wonder if that tool sees something I am not seeing (very much possible :D), or if some compilers with some optimization settings move the buffer to the heap or something? Do you have more detail about what is going on exactly here? I am really curious if it is really on the heap what is going on, as that is not something I would expect to happen here :D

Any insights would be appreciated :)

PS: btw, not saying it is such a great idea to have such large variables on the stack, but that is not the subject of this PR :D

Intriguing! My immediate thought - will making those string buffers static break thread safety?

Are there multiple drawing threads? If that is the case, then yes.

Question for you: I do not fully understand why that tool considers a stack variable "an allocation"; as that shouldn't be an allocation at all. At least, from what I see in the assembly code, these buffers don't end up on the heap, but stay on the stack. So the stack is just increased with, in this case, 2048, and the buffer sits there on the stack. That is not an "allocation" in the classical sense, as it doesn't happen on the heap.

This made me scratch my head at first, but could remember some similar situation I once ran into.
edit: This is in fact another issue.

These buffers are in fact allocated on the heap because of this:

OpenTTD/src/gfx_func.h

Line 85 in 98e653d

static const int DRAW_STRING_BUFFER = 2048;

DRAW_STRING_BUFFER is in fact not a compile time constant. There is a gcc extension that allows to use this syntax also with runtime values which allocates the buffer on the heap instead.
So it would actually be very interresting to turn the constant into a compile-time constant and see how this perfoms:

-static const int DRAW_STRING_BUFFER = 2048;
+constexpr int DRAW_STRING_BUFFER = 2048;

One can disallow compiler extensions in CMake using CXX_EXTENSIONS.

DRAW_STRING_BUFFER is a constant-expression as far as C++ is concerned. In fact, you can even do:

constexpr int x = DRAW_STRING_BUFFER;

Using static const int as array size produces the usual stack "allocation" in both gcc and clang: subtracting and then adding a fixed number to the stack pointer.

@vituscze Hi, I did some more digging regarding this and in fact you are right. Thanks for pointing that out!

I also checked the output from heaptrack again and the buffers are in fact not allocated on the heap. I simply misread the information presented by it. Part of getting used to a new tool, I guess.

@LordAro I removed the commit making the buffers static, so no worries about thread safety there.

The allocation reduction stays identical though, so the rest of the claims remain valid.

Next is the FormatString function. Currently, this function uses a stack to keep track of the remaining string.
This is not necessary as this stack can never reach 2 values. (Correct me if I am wrong.)

Doesn't look that way to me. Have you tested with something that actually uses SCC_NEWGRF_STRINL or SCC_NEWGRF_PRINT_WORD_STRING_ID? No idea what uses it though but to me it looks like it should break with this patch as it will throw away any leftover symbols after encountering those string codes.

What can probably be done though is postponing stack/vector allocation until it's actually required like how I did it in #7922

@ldpl Hmm, right.

One could also get rid of the stack by rewriting the function so that it operates on a single formatting string and recursively calls itself for these formatting codes. This makes the code cleaner, reduces the allocations and remains backwards compatible.
I'll look into that.

Will close this PR. The required refactoring won't have a lot to do with this.