Optimize StringBuilder by using LocalVector instead of Vector.

[Ivorforce]

Simple, unopinionated optimization of StringBuilder that's invisible to its callers.
This PR optimizes:

• calls like builder.append("test"), which no longer call strlen() because the size is known statically.
• as_string is now better optimized, regarding the iteration.
• LocalVector instead of Vector foregoes COW overhead (lifted from Optimize SceneTreeEditor::_update_node_tooltip() #97777).

This should slightly improve performance already, and should be easier to merge than #77158 because it does not modify any other parts of the code, and #97777 because it does not change assumptions about the strings.
I especially opted not to remove the explicit call to String(buffer, string_length) in the end because it does some error checking that should arguably stay - or at least, it should be the task of another PR to remove it.

I do think we should try optimizing strings further - especially with regards to large strings being checked for strlen quite repeatedly because of NULL termination - but this will be done in follow-up PRs. Especially, follow-up PRs will be able to call append(ptr, len) due to this PR if they already know the size.

[clayjohn]

After moving to LocalVectors, almost all of the cost of StringBuilder is from the call to String(buffer, string_length). In my test project that compiles 300 shaders I got roughly:
190 ms in master
90 ms using LocalVector instead of Vector
30 ms using String instead of raw buffer + reserving space in the local vectors

Given that, I suspect that this code will still be in the 80-90ms range, although testing is necessary to confirm of course. What error checks do you think are necessary?

[Ivorforce]

What error checks do you think are necessary?

I had a pretty in-depth look at ustring.cpp yesterday (for some reason, this discussion sucked me in). copy_from (called from the constructor) runs 3 checks that are costly:

1. Find the length of the string, clipped to p_clip_to if supplied.
2. Run a dubious cast to uint8_t, possibly preventing autovectorization.
3. Include an if branch on '\0\' in the critical loop, preventing autovectorization.

Number 2) is wholly unnecessary, as indicated by my test: onlinegdb test code. Casting rules of the C++ spec ensure this should not be different under any compiler, so we should be able to refactor the ternary with a simple static_cast<uint8_t>(p_cstr[i]). A short test indicates this check is optimized away by some compilers (e.g. clang) but not by others (MSVC).

This leaves number 1) and number 3).
Number 1) clips the string to the first '\0' character it finds.
Number 3) again checks for '\0', and if found, prints an error.

It is impossible to trigger the Number 3) check, because the 1) check will terminate the string early anyway. Though the reliance on NULL termination and repeated length checks is another discussion we should have.

To answer your question directly: After going in-depth, I don't think any of these are necessary, but whether or not they are should be discussion of other PRs.

[Ivorforce]

I removed the static-string length parts. They weren't working anyway yet, and I've been moving to a more well-rounded approach to avoiding strlen calls. In any case, this should make the PR even less contentious to merge.

[hpvb]

LGTM, straightforward improvement! Thanks!

[kiroxas]

Not sure the changes in the cpp file has any impact on performance. But changing to local Vector is definitely a good thing !

[Ivorforce]

Not sure the changes in the cpp file has any impact on performance. But changing to local Vector is definitely a good thing

You're right. I just did some more tests. I think something about using buffer pointers directly confuses the compiler. In some instances i even got worse results by removing the running index. In any case, I've just removed the .cpp changes, just to be agnostic. It should be a trivial merge now.

[clayjohn]

Let's go ahead and merge this for now as it makes a huge difference and is totally safe.

However, I do think we can go ahead with the changes from #97777

I don't think the extra validation from calling String() is relied upon, so appending directly into a String will make this way faster (in my testing it made StringBuilder essentially as fast as StringBuffer for shader compilation). Additionally, the addition of the clear() method allows us to use thread_local versions of this for places where it will be called at run time to allow us to avoid any allocations at run time (after the first few calls of course).

To be clear. I think we should merge this and then evaluate the other improvements in follow up PRs (including potentially just replacing the current implementation with what we currently have in StringBuffer)

[Repiteo]

Thanks!