Cache TreeItems between runs in EditorHelpSearch
#77471
Conversation
AThousandShips
force-pushed
the
help_search_index
branch
from
5264b8b to
8b311cf
Compare
|
Realized I can remove some checks from slice, will update that too Edit: Will also make it scroll to the first object on empty search as it otherwise keeps the original scroll which is annoying (at least without saving folding, only affects empty search), will add this change in a bit |
AThousandShips
force-pushed
the
help_search_index
branch
from
8b311cf to
b87913b
Compare
AThousandShips
changed the title
AThousandShips
force-pushed
the
help_search_index
branch
from
b87913b to
4b60116
Compare
So this is my main concern here. If we were to introduce some sort of cache, we should use it for all searches, not just for empty searches. With this PR we would be adding complexity as a temporary fix for only a part of use cases (and a small part at that). We would still need a complete solution that covers other cases, so it's likely that the code here is going to be reworked anyway. I think that it's a problem. There are also existing discrepancies in the current code regarding how we treat empty and non-empty searches, which I find weird (why are we sometimes ignoring scripts, for instance?) and which we should not aim to preserve. What are your main blockers from implementing a proper caching mechanism? And what exactly do we want to cache to speed things up? I think that main bottlenecks are filtering and building the tree. And especially for empty searches building the hierarchy of classes itself is a very small part, I think. We basically already have everything we need in So I'm thinking, hierarchy would rarely change compared to the number of searches we need to run every time a user works with the editor normally. Perhaps we should have a hierarchal structure in And with #77446 merged we could start caching I'm not sure if we can speed up filtering itself, but we can at least ensure we keep the results for a particular set of filters, so changing display options doesn't trigger pointless searches. This is where I see a thing like your index being useful. But it needs to be implemented differently, of course. Let me know what you think! PS. The implementation looks mostly fine! I would have a few nitpicks, but no point mentioning them for now since I'd prefer a different approach all together as explained above. |
|
Good points, will look into the details of this, the original issue was that filling the tree without the index was very slow in the original PR Will look into transferring the system for this to the DocData directly, shouldn't be too complicated (thinking possibly The benefit mainly for speed of having the index I believe is that it prevents adding nodes at random points in the tree at each step, as the doc order is not related to the hiearchy order Will take a deeper look at how some of this might be achieved tomorrow |
|
Minor update, based on some tentative tests:
Will open a separate PR for the doc data stuff because it touches some other aspects and feel best to get it as one step Edit: might be easier to integrate into |
|
I wouldn't touch ClassDB, unless we absolutely have to for some reason. Let's keep this localized to the editor and the docs. |
AThousandShips
force-pushed
the
help_search_index
branch
from
4b60116 to
606f5b6
Compare
AThousandShips
changed the title
TreeItems between runs in EditorHelpSearch
AThousandShips
force-pushed
the
help_search_index
branch
2 times, most recently
from
b2b8cb7 to
a1f5213
Compare
AThousandShips
force-pushed
the
help_search_index
branch
3 times, most recently
from
d573db9 to
29b4b4c
Compare
AThousandShips
force-pushed
the
help_search_index
branch
from
29b4b4c to
752c7be
Compare
AThousandShips
force-pushed
the
help_search_index
branch
from
752c7be to
227a967
Compare
AThousandShips
force-pushed
the
help_search_index
branch
from
227a967 to
525fe5c
Compare
|
So I did some performance testing with this patch, and there are some improvements, but I would say not where it matters the most. This suggests that the biggest task during the search is, well, the search itself, the filtering. So tree optimizations don't help a lot. Here's what I've got. This PR but with _find_or_create_item always creating itemI then properly reverted to the base commit (your pre-requisite PR) and repeated the test, numbers were pretty much the same. This PR as isMy testing routine was to open and close it a few times, then open it and type "CanvasItem" then select all and delete it, also a few times. You can see that:
Benchmarking is done with this patch: diff --git a/editor/editor_help_search.cpp b/editor/editor_help_search.cpp
index afe2f97f47..b62c9cca10 100644
--- a/editor/editor_help_search.cpp
+++ b/editor/editor_help_search.cpp
@@ -48,6 +48,7 @@ void EditorHelpSearch::_update_results() {
search_flags |= SEARCH_SHOW_HIERARCHY;
}
+ search_started = OS::get_singleton()->get_ticks_usec();
search = Ref<Runner>(memnew(Runner(results_tree, results_tree, &tree_cache, term, search_flags)));
set_process(true);
}
@@ -128,6 +129,8 @@ void EditorHelpSearch::_notification(int p_what) {
if (search.is_valid()) {
if (search->work()) {
// Search done.
+ uint64_t search_ended = OS::get_singleton()->get_ticks_usec();
+ print_verbose(vformat("EditorHelp search done in %s usec", search_ended - search_started));
// Only point to the match if it's a new search, and not just reopening a old one.
if (!old_search) {
diff --git a/editor/editor_help_search.h b/editor/editor_help_search.h
index e4980d6ff7..e2afa6ee88 100644
--- a/editor/editor_help_search.h
+++ b/editor/editor_help_search.h
@@ -66,6 +66,7 @@ class EditorHelpSearch : public ConfirmationDialog {
class Runner;
Ref<Runner> search;
+ uint64_t search_started = 0;
struct TreeCache {
HashMap<String, TreeItem *> item_cache;
|
| r_item = tree_cache->item_cache[p_item_meta]; | ||
|
|
||
| // Remove from cache. | ||
| tree_cache->item_cache.erase(p_item_meta); |
There was a problem hiding this comment.
I wasn't sure why we would clear the cache here, since the HashMap should update existing items anyway on insert. But removing this line doesn't introduce any performance improvements (and I may have introduced a leak with this, didn't debug much).
There was a problem hiding this comment.
It would risk double deleting items on close as any item already in the tree is deleted when clearing it, but might work out as it's deferred I realize, but unsure
There was a problem hiding this comment.
Well, doesn't matter if removing it doesn't help. Just thought I'd mention it.
|
Once I've pushed some changes you can also compare the same performance with the complete PR, where I hope it'll help more with larger empty searches 🙂, probably areas where more can be shaved off as well |
AThousandShips
force-pushed
the
help_search_index
branch
from
525fe5c to
4e9e649
Compare
|
And thank you for verifying the performance, wasn't sure how I'd do it rigorously myself |
AThousandShips
force-pushed
the
help_search_index
branch
from
4e9e649 to
2920a87
Compare
|
Did some further performance checking and realized that the clearing of the tree also matters, so these are my results, using: diff --git a/editor/editor_help_search.cpp b/editor/editor_help_search.cpp
index 229eb79e11..dd4d6ebbdc 100644
--- a/editor/editor_help_search.cpp
+++ b/editor/editor_help_search.cpp
@@ -48,6 +48,7 @@ void EditorHelpSearch::_update_results() {
search_flags |= SEARCH_SHOW_HIERARCHY;
}
+ search_started = OS::get_singleton()->get_ticks_usec();
search = Ref<Runner>(memnew(Runner(results_tree, results_tree, &tree_cache, term, search_flags)));
set_process(true);
}
@@ -128,6 +129,8 @@ void EditorHelpSearch::_notification(int p_what) {
if (search.is_valid()) {
if (search->work()) {
// Search done.
+ uint64_t search_ended = OS::get_singleton()->get_ticks_usec();
+ print_verbose(vformat("EditorHelp search done in %s usec", search_ended - search_started));
// Only point to the match if it's a new search, and not just reopening a old one.
if (!old_search) {
@@ -445,6 +448,8 @@ bool EditorHelpSearch::Runner::_phase_match_classes() {
}
void EditorHelpSearch::Runner::_populate_cache() {
+ results_tree->clear();
+
root_item = results_tree->get_root();
if (root_item) {
@@ -659,7 +664,7 @@ TreeItem *EditorHelpSearch::Runner::_create_class_hierarchy(const ClassMatch &p_
bool EditorHelpSearch::Runner::_find_or_create_item(TreeItem *p_parent, const String &p_item_meta, TreeItem *&r_item) {
// Attempt to find in cache.
- if (tree_cache->item_cache.has(p_item_meta)) {
+ if (false && tree_cache->item_cache.has(p_item_meta)) {
r_item = tree_cache->item_cache[p_item_meta];
// Remove from cache.
diff --git a/editor/editor_help_search.h b/editor/editor_help_search.h
index e4980d6ff7..e2afa6ee88 100644
--- a/editor/editor_help_search.h
+++ b/editor/editor_help_search.h
@@ -66,6 +66,7 @@ class EditorHelpSearch : public ConfirmationDialog {
class Runner;
Ref<Runner> search;
+ uint64_t search_started = 0;
struct TreeCache {
HashMap<String, TreeItem *> item_cache;For the first, and just remove the No caching, clears tree (this is really what should be compared with, or we'll have leaking objects) No caching, no clearing of tree Caching I'd appropriate a comparison by you too @YuriSizov if you're able, because it looks like your results don't take this into account from your description and stats |
Well, as noted I also checked times without this commit completely, and they were in the same ballpark. I think what you demonstrate here aligns with my numbers. Overall you get a ~45-50% improvement on "empty" searches, but the rest of the runs remain consistent with or without the patch. For you it's 20-25k usec, for me it was 30-35k usec. Unless you see something else in this data? |
|
My bad missed the note that the data was the same, noticed some increase with the clear applied but it's largely equivalent, it was more pronounced on the final one and didn't test as much here and it's largely negligible, it did seem more pronounced especially when clearing a very large tree (see the first search after empty) 🙂 |
|
Thanks! |
|
Thank you! |
Split off from #62524
Depends on #77554, which it includes