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

Improve Tree performance #94748

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Improve Tree performance #94748

merged 1 commit into from
Aug 27, 2024

Conversation

aaronp64
Copy link
Contributor

@aaronp64 aaronp64 commented Jul 25, 2024

Added TreeItem::last_child to avoid needing to iterate through all children to get to the end. This mainly helps in cases where one TreeItem has many children (1000s), and new children are added to the end, as each add had to iterate through all previously added children.

Gdscript example to compare creating TreeItems, time reduced from 1640ms to 66ms:

func _ready() -> void:
	var root = $Tree.create_item()
	root.set_text(0, "Root")
	var start_time = Time.get_ticks_msec()
	for i in 20000:
		var item = $Tree.create_item()
		item.set_text(0, "Item " + str(i))
	var end_time = Time.get_ticks_msec()
	print("Time: %dms" % (end_time - start_time))

This also impacts SceneTreeEditor. I created a Node2D scene with 10,000 empty Node2D children to test with. Opening this scene took 4.28 seconds before this change, 1.55 seconds after. At 20,000 children, time was reduced from 16 seconds to 2.65 seconds.

EditorDebuggerTree has similar slowness in adding items, but also is repeatedly updated when running game from the editor. Using the gdscript below to create 30,000 Node2Ds at runtime, the editor is significantly more responsive when using the remote scene tree while the parent node stays collapsed. Once the parent node is expanded, more time is spent handling the tree's text/heights, which gets pretty slow, but is still faster than before.

func _ready() -> void:
	for i in 30000:
		add_child(Node2D.new())

Also added some unit tests. All tests pass with both old and new code, except for the Clear items test, which failed on old code due to children_cache not being cleared in clear_children(). Looks like we avoided needing to use the cache in existing code where clear_children() is called, so this hasn't been an issue.

@aaronp64 aaronp64 requested review from a team as code owners July 25, 2024 18:54
@clayjohn clayjohn added this to the 4.4 milestone Jul 25, 2024
@clayjohn clayjohn requested a review from KoBeWi July 25, 2024 19:15
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

Testing project: test-tree.zip

Benchmark

Using an optimized editor build (MSVC 2022 optimize=speed). Running the MRP above which contains the first code sample from OP.

Before After
745 ms 38 ms
PC specifications
  • CPU: Intel Core i9-13900K
  • GPU: NVIDIA GeForce RTX 4090
  • RAM: 64 GB (2×32 GB DDR5-5800 C30)
  • SSD: Solidigm P44 Pro 2 TB
  • OS: Windows 11 23H2

@@ -121,6 +121,7 @@
#include "tests/scene/test_sprite_frames.h"
#include "tests/scene/test_theme.h"
#include "tests/scene/test_timer.h"
#include "tests/scene/test_tree.h"
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved down to the #ifndef ADVANCED_GUI_DISABLED part as it is advanced UI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to advanced gui section

@Jimmio92
Copy link

Such a shame this came too late for 4.3; speed improvements to scene addition across the board like this are a huge help to those developing/attempting larger titles with Godot. Thanks for finding this.

Added TreeItem::last_child to avoid needing to iterate through all children to get to the end.  This mainly helps in cases where one TreeItem has many children (1000s), and new children are added to the end, as each add had to iterate through all previously added children.
@Mickeon
Copy link
Contributor

Mickeon commented Jul 26, 2024

It can probably be cherrypicked later, assuming lack of unforeseen consequences

@Saul2022
Copy link

Wouldn't this fix #94648 (comment) along with rthe closing pr?

@aaronp64
Copy link
Contributor Author

Wouldn't this fix #94648 (comment) along with rthe closing pr?

No, I ran into this when looking into that issue, but most of the time spent in that case is in DynamicBVH functions. From their screenshots, I think the reason Tree wasn't an issue there is that the block nodes were internal to the map, so they weren't being added to the SceneTreeEditor

@akien-mga akien-mga merged commit 9e1c63a into godotengine:master Aug 27, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member

AThousandShips commented Aug 28, 2024

This broke trees in at least EditorHelpSearch, will try to identify what was broken

Edit: Fixed, see:

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.

9 participants