Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize SceneTreeEditor::_update_node_tooltip() #97777

Closed
wants to merge 4 commits into from

Conversation

YYF233333
Copy link
Contributor

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)

master

This PR: (about 9.0s)

pr

This PR without changes applied to StringBuilder: (about 9.5s)

sb

@YYF233333 YYF233333 requested review from a team as code owners October 3, 2024 16:35
@AThousandShips AThousandShips added this to the 4.x milestone Oct 3, 2024
@YYF233333 YYF233333 force-pushed the stringbuilder branch 2 times, most recently from fca248d to c1e507d Compare October 4, 2024 04:45
@YYF233333
Copy link
Contributor Author

restore the original as_string() method, something in the mono interface depends on const attribute.

@YYF233333 YYF233333 requested a review from a team as a code owner November 5, 2024 13:49
Comment on lines +525 to +529
tooltip += "\n";
tooltip += TTR("Inherits:");
tooltip += " ";
tooltip += p_node->get_scene_inherited_state()->get_path();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for now, but this makes me think we should have some vararg StringBuilder method for appending multiple strings at once.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't measure performance, but functionally it works correctly.
AFAIK using LocalVector in StringBuffer was an already planned optimization

@YYF233333
Copy link
Contributor Author

Seems no more review, squash commits and rebase against master.

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Nov 20, 2024
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! The timing is wonderful too, I was just noticing the cost of StringBuilder in the ShaderCompiler. I tested this locally and confirmed that it already helps there (without even taking advantage of using a static version of StringBuilder).

In follow up PRs we should be using a static StringBuilder in more places

@clayjohn
Copy link
Member

Have you had a chance to look at the improvements you mentioned in #77158 (comment)? 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

Co-authored-by: Tomek <kobewi4e@gmail.com>
@YYF233333
Copy link
Contributor Author

@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.

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@clayjohn
Copy link
Member

@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

@Ivorforce
Copy link
Member

Ivorforce commented Dec 12, 2024

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.

@YYF233333
Copy link
Contributor Author

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.

@YYF233333 YYF233333 closed this Dec 12, 2024
@YYF233333 YYF233333 deleted the stringbuilder branch December 12, 2024 08:21
@AThousandShips AThousandShips removed this from the 4.4 milestone Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants