Optimize classnames enumeration #101489

RandomShaper · 2025-01-13T12:54:50Z

Hereby, a number of items involved in presenting icons of global classes are rewired to leverage the global class cache much better. As a consequence, the time it takes for the node create dialog and autocomplete popup to show up (especially in cold, just after opening the editor) is reduced considerably. This is especially impactful in big projects. I've been a witness of a moderately big project taking about 10 seconds for the node create dialog to be shown for it.

This PR also makes an initial commit that simplifies file system cache parsing, something one of the other commits leverages when it comes to including more attributes in the global class cache.

Benchmarking on a test project with thousands of trivial globally named GDScript and C# classes, some of them with icons, these are the results:

Without this PR
- Create node dialog
  Cold: 4692 ms (icon loads: 10, script loads: 2268)
  Hot: 1810 ms (icon loads: 0, script loads: 0)
- Autocomplete popup
  Cold: 3079 ms (icon loads: 10, script loads: 756)
  Hot: 698 ms (icon loads: 0, script loads: 0)
With this PR
- Create node dialog
  Cold: 182 ms (icon loads: 10, script loads: 0
  Hot: 172 ms (icon loads: 0, script loads: 0)
- Autocomplete popup
  Cold: 91 ms (icon loads: 10, script loads: 0)
  Hot: 83 ms (icon loads: 0, script loads: 0)

As the data above shows, one of the reasons for the speedup is that the new implementation doesn't need to load (potentially multiple times) all the scripts involved. The number of icon loads was measures as well to ensure no regressions in that area were caused by this PR. That is, the editor script icon cache is still used as most as possible.

Besides everything explained above, this PR removes part of the extra work the involved code was doing (such as checking the deprecated custom types' icons), to behave more consistently with what the file system dock does. Given the extent of the changes, there's some risk of regressions in picking up icons for classes.

Version of this PR for 4.3 submitted as #101490.

UPDATE: I forgot to add that another potential regression is that the create node dialog won't be able to show the global classes before they have been parsed and cached, which is especially true in the cold case. If that's true and a concern, we could still fall back to parsing the script files or let the create dialog wait until the global classes cache is ready.

Production edit: See Script classes make the CreateDialog search filter prohibitively slow #27333.

Fixes #101194

editor/editor_file_system.cpp

akien-mga · 2025-01-13T13:04:48Z

editor/editor_file_system.cpp

-#define CACHE_FILE_NAME "filesystem_cache8"
+#define CACHE_FILE_NAME "filesystem_cache10"


Do we follow Microsoft versioning going from 8 to 10? :P

In the version for 4.3, I raised to filesystem_cache9. In case someone switches between 4.3 and 4.4 for some reason to edit a single project, that'd ensure compatibility. Or that was my reasoning at least.

I guess that if the change is the same in both 4.3 and 4.4, and both were on version 8, they could both use version 9 and stay compatible.

Otherwise if we do have an incompatibility between the two implementations I agree they should use different numbers. It's bikeshedding, but I may argue it might be better for the master version to stay sequential, and for potential feature backports to take care of special casing compatibility, e.g. by introducing a suffix system for version changes in stable branches (e.g. filesystem_cache8b). But that's really nitpicky and not a hill I'd die on :P I'm also fine with 9/10.

The only (very theoretical) case where the current approach could be problematic is if we then make another version change in both 4.3 and 4.4, and we need to find where to go from 9 and 10. We'd end up with something like 9 -> 11 in 4.3 and 10 -> 12 in 4.4 or similar, which starts to be a bit weird.

Maybe we could just fork both forever in a way that they can't clash. More detailedly, we'd use something like filesystem_cache0001 for master and filesystem_cache1001 for 4.3.

Alternatively, we can backport #100729, which is the one adding a need for checking 'is tool` so we can keep both file system cache formats the same for now...

akien-mga

Looks pretty good to me overall from reading the code. Haven't tested.

akien-mga · 2025-01-13T13:07:52Z

core/object/script_language.cpp

@@ -390,7 +390,7 @@ void ScriptServer::global_classes_clear() {
 	inheriters_cache.clear();
 }

-void ScriptServer::add_global_class(const StringName &p_class, const StringName &p_base, const StringName &p_language, const String &p_path) {
+void ScriptServer::add_global_class(const StringName &p_class, const StringName &p_base, const StringName &p_language, const String &p_path, bool p_is_abstract, bool p_is_tool) {


I don't have a particular suggestion for improvement, but I can't help wonder if there won't be other types of qualifiers we need to keep track on in the future, leading to further boolean parameters. It's probably fine, though the GDExtension compatibility might need some care.

Since I've added just two, I've kept them as separate args here and as separate fields in the classnames cache. For future extensions, I'd say moving to some kind of bitset or similar would be advisable. I'll check the GDExtension compat.

akien-mga · 2025-01-13T13:08:54Z

core/object/script_language_extension.h

@@ -672,7 +672,7 @@ class ScriptLanguageExtension : public ScriptLanguage {

 	GDVIRTUAL1RC_REQUIRED(Dictionary, _get_global_class_name, const String &)

-	virtual String get_global_class_name(const String &p_path, String *r_base_type = nullptr, String *r_icon_path = nullptr) const override {
+	virtual String get_global_class_name(const String &p_path, String *r_base_type = nullptr, String *r_icon_path = nullptr, bool *r_is_abstract = nullptr, bool *r_is_tool = nullptr) const override {


These should likely be added to the GDVIRTUAL_CALL, otherwise GDExtension scripting languages won't be able to pass this information.

@dsnopek just added a system for virtual methods compatibility bindings which should work here: #100674.

I'm always—inexplicably—fuzzy about this GDExtension compatibility topic, like I used to be with GDNative... So, let's see if I'm getting this straight. This is the only related binding I'm being able to find: GDVIRTUAL_BIND(_get_global_class_name, "path");

Given it only cares about the first parameter, looks like we are lucky that second parameter onwards don't have an impact. Is that right?

akien-mga · 2025-01-13T13:13:09Z

CC @YYF233333 who has been working on #101435 recently, which seems partially related (both are probably relevant to merge but good to cross-review).

YYF233333 · 2025-01-13T13:47:52Z

This one change the sign of EditorNode::get_object_icon as well as some change I don't come up with. It probably won't resolve RemoteTree performance issue though, because we are still doing ResourceLoader::exists() for every icon, so mine is still relevant. I think I can rebase on top of this once it is settled.

YYF233333 · 2025-01-15T07:46:48Z

Just asking, since we reaches beta and this one has a milestone 4.x, is it planned to catch up with 4.4? If not, I think mine can go ahead to privide a quick fix for #78645 and #98666.

Calinou

Tested locally, it works as expected. Code looks good to me.

Testing project: test_class_names.zip
This project includes a generate.sh script that can be used to create projects with lots of class_names with custom icons.¹

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: Linux (Fedora 41)

Using an optimized (LTO) editor build.

In a project with 10,000 named classes that have their own icon each, this is the delay before the Create New Node dialog shows up after clicking the button:

Cold:

Before	After
4.1 seconds	1.6 seconds

Warm:

Before	After
0.9 seconds	0.3 seconds

Same project without the pregenerated classes in case you have trouble extracting it: test_class_names.zip ↩

KoBeWi · 2025-01-22T12:23:20Z

Unfortunately this breaks custom icons for scripts that aren't global classes. @icon is supposed to work in any script, even built-in ones. It affects Scene dock.

EDIT:
Custom type icons (added via add_custom_type()) also broke. Although custom types were pretty much replaced by class_name by now, so it's less relevant. We still need to deprecate them properly though.

KoBeWi · 2025-01-22T12:30:01Z

core/object/script_language.cpp

+	ERR_FAIL_COND_V(!global_classes.has(p_class), false);
+	return global_classes[p_class].is_abstract;


Suggested change

ERR_FAIL_COND_V(!global_classes.has(p_class), false);

return global_classes[p_class].is_abstract;

GlobalScriptClass *global_class = global_classes.getptr(p_class);

ERR_FAIL_COND_V(!global_class, false);

return global_class->is_abstract;

Same below.

KoBeWi · 2025-01-22T12:31:37Z

core/object/script_language.cpp

@@ -513,6 +527,8 @@ void ScriptServer::save_global_classes() {
 		d["path"] = global_classes[E].path;
 		d["base"] = global_classes[E].base;
 		d["icon"] = class_icons.get(E, "");
+		d["is_abstract"] = global_classes[E].is_abstract;


It would be better if global_classes[E] was performed only once.

const GlobalScriptClass &global_class = global_classes[E];

KoBeWi · 2025-01-22T14:05:55Z

editor/editor_file_system.h

@@ -312,7 +326,7 @@ class EditorFileSystem : public Node {
 	void _update_pending_scene_groups();
 	void _get_all_scenes(EditorFileSystemDirectory *p_dir, HashSet<String> &r_list);

-	String _get_global_script_class(const String &p_type, const String &p_path, String *r_extends, String *r_icon_path) const;
+	EditorFileSystem::ScriptClassInfo _get_global_script_class(const String &p_type, const String &p_path) const;


Suggested change

EditorFileSystem::ScriptClassInfo _get_global_script_class(const String &p_type, const String &p_path) const;

ScriptClassInfo _get_global_script_class(const String &p_type, const String &p_path) const;

KoBeWi · 2025-01-22T14:07:10Z

editor/editor_file_system.cpp

-				if (fc->type.is_empty()) {
+				if (fc->type == StringName()) {


StringName does have is_empty() method, so this change is unnecessary.

KoBeWi · 2025-01-22T14:27:16Z

editor/editor_data.h

@@ -255,9 +254,9 @@ class EditorData {
 	void script_class_save_icon_paths();
 	void script_class_load_icon_paths();

-	Ref<Texture2D> extension_class_get_icon(const String &p_class) const;
+	String extension_class_get_icon_path(const String &p_class) const;


What's this change for? How is returning path better if you load it anyway?

RandomShaper added topic:editor performance labels Jan 13, 2025

RandomShaper added this to the 4.x milestone Jan 13, 2025

RandomShaper requested review from a team as code owners January 13, 2025 12:54

RandomShaper mentioned this pull request Jan 13, 2025

[4.3] Optimize classnames enumeration #101490

Open

akien-mga added the enhancement label Jan 13, 2025

akien-mga reviewed Jan 13, 2025

View reviewed changes

editor/editor_file_system.cpp Outdated Show resolved Hide resolved

akien-mga reviewed Jan 13, 2025

View reviewed changes

RandomShaper force-pushed the optimize_classnames_enumeration branch from 9cddbcf to 462966e Compare January 13, 2025 13:06

RandomShaper requested a review from a team as a code owner January 13, 2025 13:06

akien-mga reviewed Jan 13, 2025

View reviewed changes

akien-mga requested review from KoBeWi and Hilderin January 13, 2025 13:11

RandomShaper force-pushed the optimize_classnames_enumeration branch from 462966e to 726715a Compare January 16, 2025 17:59

RandomShaper added 3 commits January 21, 2025 12:59

Rationalize parsing of file system cache

aca5cfc

Include more attributes in the global class names cache

3881b38

Optimize enumeration of global classes in create dialog and autocomplete

1b07897

RandomShaper force-pushed the optimize_classnames_enumeration branch from 726715a to 1b07897 Compare January 21, 2025 12:08

Calinou approved these changes Jan 21, 2025

View reviewed changes

KoBeWi reviewed Jan 22, 2025

View reviewed changes

KoBeWi mentioned this pull request Jan 24, 2025

Refactor EditorNode get icon #101435

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize classnames enumeration #101489

Optimize classnames enumeration #101489

RandomShaper commented Jan 13, 2025 •

edited by KoBeWi

Loading

akien-mga Jan 13, 2025

RandomShaper Jan 13, 2025

akien-mga Jan 13, 2025 •

edited

Loading

RandomShaper Jan 13, 2025

akien-mga left a comment

akien-mga Jan 13, 2025

RandomShaper Jan 13, 2025

akien-mga Jan 13, 2025

RandomShaper Jan 13, 2025

akien-mga commented Jan 13, 2025

YYF233333 commented Jan 13, 2025

YYF233333 commented Jan 15, 2025

Calinou left a comment

KoBeWi commented Jan 22, 2025 •

edited

Loading

KoBeWi Jan 22, 2025

KoBeWi Jan 22, 2025

KoBeWi Jan 22, 2025

KoBeWi Jan 22, 2025

KoBeWi Jan 22, 2025

		#define CACHE_FILE_NAME "filesystem_cache8"
		#define CACHE_FILE_NAME "filesystem_cache10"

		ERR_FAIL_COND_V(!global_classes.has(p_class), false);
		return global_classes[p_class].is_abstract;

-	ERR_FAIL_COND_V(!global_classes.has(p_class), false);
-	return global_classes[p_class].is_abstract;
+	GlobalScriptClass *global_class = global_classes.getptr(p_class);
+	ERR_FAIL_COND_V(!global_class, false);
+	return global_class->is_abstract;

	EditorFileSystem::ScriptClassInfo _get_global_script_class(const String &p_type, const String &p_path) const;
	ScriptClassInfo _get_global_script_class(const String &p_type, const String &p_path) const;

Optimize classnames enumeration #101489

Are you sure you want to change the base?

Optimize classnames enumeration #101489

Conversation

RandomShaper commented Jan 13, 2025 • edited by KoBeWi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Jan 13, 2025

YYF233333 commented Jan 13, 2025

YYF233333 commented Jan 15, 2025

Calinou left a comment

Choose a reason for hiding this comment

Footnotes

KoBeWi commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomShaper commented Jan 13, 2025 •

edited by KoBeWi

Loading

akien-mga Jan 13, 2025 •

edited

Loading

KoBeWi commented Jan 22, 2025 •

edited

Loading