godotengine
Optimize `SceneTreeEditor::_update_node_tooltip()`
split to 2 commit to make review easier.
Make SceneTreeEditor::_update_node_tooltip() reuse the string build buffer instead of alloc buffer/tmp string everytime.
1st commit: make StringBuilder suitable for multi time use by reusing the buffer in as_string() method.
2nd commit: use a static StringBuilder for string concat.
performance:
bench with ManyNodes.zip, test time to open ManyNodes.tscn (100k nodes).
Master branch: (about 9.5s)
This PR: (about 9.0s)
This PR without changes applied to StringBuilder: (about 9.5s)
restore the original as_string() method, something in the mono interface depends on const attribute.
Have you had a chance to look at the improvements you mentioned in https://github.com/godotengine/godot/pull/77158#issuecomment-2492088929? I ran some quick tests this weekend and compared the performance of #77158 against this PR (using a String internally) when compiling shaders and #77158 still seems to be faster by a small margin
@clayjohn Can you share your test project? The one I used above cannot show difference anymore (some Tree optimizations definitely get merged).
The small difference you measured should probably be Small String Optimization, I'm seeking to introduce that to String, but having problem with COW semantics.
@YYF233333 Here is my test scene. It waits one second, then compiles 300 shaders. It prints the time each frame so you can get a rough sense of how long the overall compilation took.
To get the timing of StringBuilder, I ran this demo with a sampling profiler attached so I could see the cost of StringBuilder in particular. ShaderCompilationIssue.zip
I have submitted #100295 to separately merge buffer changes in StringBuilder. It leads to massive speed ups already. In my opinion, the use of static and / or threadlocal StringBuilder should be checked separately for their contribution to speed ups.
One reason to avoid static mutables is because they can lead to multithreading pitfalls.
One reason to avoid threadlocal variables is because they can be less efficient than stack-local variables.
If you disagree, feel free to merge this PR and close #100295, as it does not add any additional functionality not also added here.
Close this in favour of #100295. update_node_tooltip part should be reconsidered after concat_string is merged, that one lead to more readable code, and this function is not that performance critical. I see LocalVector has been applied already, so the remaining useful part of this PR is identical with #100295.