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

Scripting: Fix script docs not being searchable without manually recompiling scripts #95821

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anvilfolk
Copy link
Contributor

@anvilfolk anvilfolk commented Aug 19, 2024

This PR adds a script documentation cache in the project folder. It is loaded at alongside native documentation caches. This makes scripts fully accessible through Search Help, including their members, etc, right from project start, without having to compile every single script manually to access its docs.

Testing with GDScript but, in theory, should work for any scripting language that has documentation support. Might be worth splitting the cache into a per-language basis to reduce conflicts, though if there are conflicts with e.g. a MyClass in GDScript and MyClass in C#, that may already lead to problems with the current documentation system.

Ensuring file deletions when Godot closed trigger EditorFileSystem's documentation updates is done by #95965, until then the cache will persist docs from deleted scripts Done.

Diagram of call behavior that I needed to try to organize this in my brain:

image

Grey background runs on whatever thread called it. Orange background runs in worker_thread. Green background runs on loader_thread, and can include ResourceLoader::load() calls, orange cannot. Blue background represents waiting for one shot sources_updated signals depending on whether EditorFileSystem::is_scanning() is true.

Implementation details:

  • Doc tasks that don't use ResourceLoader::load() to generate docs always happens in EditorHelp::worker_thread.
  • Regenerating script docs using ResourceLoader::load() happen in EditorHelp::loader_thread. This prevents deadlocks where the main thread needs doc info and waits for worker_thread, but worker_thread is waiting for main thread to dispatch loading tasks.
  • Script cache should not reload when GDExtension docs change, overwriting doc changes of the current session ❗ someone with GDExtensions please test!❗ .
  • If cache isn't present, worker_thread connects to EditorFileSystem::filesystem_changed signal, indicating the end of EditorFileSystem's scan, and ends its execution. It spools back up once the signal is fired to regenerate the cache, this time using EditorFileSystemDirectory, therefore not accessing underlying OS filesystem calls.
  • Cache is saved & loaded as-is in memory or in the disk. We let EditorFileSystem catch changes like deleted/added files to ensure docs are kept up to date.
  • Cache only saves on exit, since it requires bulk-saving every script doc to disk as a Resource. That feels like too much to do every time a script is saved/compiled to keep disk & memory docs consistent.
  • Added DocTool method forwarding to EditorHelp to help maintain cache consistency. It is meant to track whether changes affect script docs or other docs. These forwarded methods should be used from now on instead of e.g. get_doc_data()->add_doc().
  • Disk cache is deleted after successful load. It saves on editor exit, if successfully loaded/regenerated previously.
  • Somewhat unrelated but removed quotes around documentation pages in the script list, e.g. "new_script.gd" becomes new_script.gd, since this resulted in messy truncations of paths (to test, create new_folder/new_script.gd and new_script.gd and open both their documentations on stable.

Fixes #72406, fixes #86577, fixes #72952 (for GDScript, C# docs are not supported yet).

Added coauthorship with @Hilderin because of their wonderful and insightful help in designing and testing this <3

editor/editor_help.cpp Outdated Show resolved Hide resolved
@dalexeev
Copy link
Member

  • Somewhat unrelated but removed quotes around documentation pages in the script list, e.g. "new_script.gd" becomes new_script.gd, since this resulted in messy truncations of paths (to test, create new_folder/new_script.gd and new_script.gd and open both their documentations on stable.

I think it's better to fix this by checking the tab type. Doc tabs should never truncate the name, only script tabs.

@anvilfolk
Copy link
Contributor Author

  • Somewhat unrelated but removed quotes around documentation pages in the script list, e.g. "new_script.gd" becomes new_script.gd, since this resulted in messy truncations of paths (to test, create new_folder/new_script.gd and new_script.gd and open both their documentations on stable.

I think it's better to fix this by checking the tab type. Doc tabs should never truncate the name, only script tabs.

Why the difference? You can still get the full path by hovering and it's at the top of the help document. This way it feels like behavior is at least consistent?

@anvilfolk anvilfolk marked this pull request as ready for review August 23, 2024 18:31
@anvilfolk anvilfolk requested a review from a team as a code owner August 23, 2024 18:31
@anvilfolk anvilfolk changed the title Scripting: Add script documentation cache to project Scripting: fix script docs not being searchable without manually recompiling scripts Aug 23, 2024
editor/editor_help.cpp Outdated Show resolved Hide resolved
@anvilfolk
Copy link
Contributor Author

Yeah but until you modify a script, the cache is valid.

Yes! But it was already loaded on startup, so it did its job. My claims are these:

  1. It's unlikely someone will use Godot without touching any script (thus invalidating the cache), so keeping the cache is useful only in few usecases.
  2. The cache saves quickly on exit, so it's not worth the extra code just for the few usecases above.
  3. If performance is an issue for large projects, it will be an issue in most uses of the editor (when scripts are changed) so we need a different solution anyways, e.g. project setting to disable script docgen.

Also no reason to delete the file - you can just overwrite it when saving, no?

We delete it so we don't load docs inconsistent with the scripts if Godot crashes and you don't get to save the disk cache on editor exit. Rewriting the entire cache any time a script is saved feels overkill and a performance issue! :)

@KoBeWi
Copy link
Member

KoBeWi commented Sep 23, 2024

Tested with my very big project™ and the editor is stuck on reopening scenes:
image
It's one of the initial steps when launching. Maybe something gets locked by the thread?

@anvilfolk
Copy link
Contributor Author

anvilfolk commented Sep 23, 2024

I also noticed I am getting core\object\object.cpp:2342 - ObjectDB instances leaked at exit (run with --verbose for details). when the cache is regenerated. I'll need to test with a smaller project so I can use --verbose.

Thanks for testing on a large project, I only tested on project that had a lot of .gd files, not scenes. I thought most of the threading issues would've been resolved but I'll take another look.

That could be where it pauses if it's regenerating the cache - but it sounds like it's a full stop. Could be any call to EditorHelp::get_doc_data() during the initial filesystem scan 🤔

@KoBeWi
Copy link
Member

KoBeWi commented Sep 23, 2024

Stuck trace:

Thread::wait_to_finish() Line 87 (c:\godot_source\core\os\thread.cpp:87)
EditorHelp::_wait_for_thread() Line 2866 (c:\godot_source\editor\editor_help.cpp:2866)
EditorHelp::get_doc_data() Line 3329 (c:\godot_source\editor\editor_help.cpp:3329)
Shader::get_shader_uniform_list(List<PropertyInfo,DefaultAllocator> * p_params, bool p_get_groups) Line 177 (c:\godot_source\scene\resources\shader.cpp:177)
ShaderMaterial::_get_property_list(List<PropertyInfo,DefaultAllocator> * p_list) Line 234 (c:\godot_source\scene\resources\material.cpp:234)
ShaderMaterial::_get_property_listv(List<PropertyInfo,DefaultAllocator> * p_list, bool p_reversed) Line 95 (c:\godot_source\scene\resources\material.h:95)
Object::get_property_list(List<PropertyInfo,DefaultAllocator> * p_list, bool p_reversed) Line 512 (c:\godot_source\core\object\object.cpp:512)
Resource::duplicate_for_local_scene(Node * p_for_scene, HashMap<Ref<Resource>,Ref<Resource>,HashMapHasherDefault,HashMapComparatorDefault<Ref<Resource>>,DefaultTypedAllocator<HashMapElement<Ref<Resource>,Ref<Resource>>>> & p_remap_cache) Line 289 (c:\godot_source\core\io\resource.cpp:289)
SceneState::make_local_resource(Variant & p_value, const SceneState::NodeData & p_node_data, HashMap<Ref<Resource>,Ref<Resource>,HashMapHasherDefault,HashMapComparatorDefault<Ref<Resource>>,DefaultTypedAllocator<HashMapElement<Ref<Resource>,Ref<Resource>>>> & p_resources_local_to_sub_scene, Node * p_node, const StringName p_sname, HashMap<Ref<Resource>,Ref<Resource>,HashMapHasherDefault,HashMapComparatorDefault<Ref<Resource>>,DefaultTypedAllocator<HashMapElement<Ref<Resource>,Ref<Resource>>>> & p_resources_local_to_scene, int p_i, Node * * p_ret_nodes, SceneState::GenEditState p_edit_state) Line 649 (c:\godot_source\scene\resources\packed_scene.cpp:649)
SceneState::instantiate(SceneState::GenEditState p_edit_state) Line 369 (c:\godot_source\scene\resources\packed_scene.cpp:369)
PackedScene::instantiate(PackedScene::GenEditState p_edit_state) Line 2171 (c:\godot_source\scene\resources\packed_scene.cpp:2171)
SceneState::instantiate(SceneState::GenEditState p_edit_state) Line 232 (c:\godot_source\scene\resources\packed_scene.cpp:232)
PackedScene::instantiate(PackedScene::GenEditState p_edit_state) Line 2171 (c:\godot_source\scene\resources\packed_scene.cpp:2171)
EditorNode::load_scene(const String & p_scene, bool p_ignore_broken_deps, bool p_set_inherited, bool p_clear_errors, bool p_force_open_imported, bool p_silent_change_tab) Line 4045 (c:\godot_source\editor\editor_node.cpp:4045)
EditorNode::_load_open_scenes_from_config(Ref<ConfigFile> p_layout) Line 5370 (c:\godot_source\editor\editor_node.cpp:5370)
EditorNode::_load_editor_layout() Line 5270 (c:\godot_source\editor\editor_node.cpp:5270)

idk if it ever stops, I gave up after 8 mins.

EDIT:
Planting a fake cache "fixes" it, but it's only a workaround.

EDIT2:
I saved the cache as .tres and inspected it. It seems to also contain shaders?

{
"is_script_doc": true,
"name": "res://Nodes/Environment/Liquids/Water.gdshader",
"properties": [{
"name": "shader_parameter/blue_tint",
"overridden": false
}, {
"name": "shader_parameter/surface",
"overridden": false
}, {
"name": "shader_parameter/sprite_scale",
"overridden": false
}, {
"name": "shader_parameter/position",
"overridden": false
}, {
"name": "shader_parameter/scale_x",
"overridden": false
}, {
"name": "shader_parameter/flow",
"overridden": false
}, {
"name": "shader_parameter/dissort_speed",
"overridden": false
}, {
"name": "shader_parameter/dissort_scale",
"overridden": false
}, {
"name": "shader_parameter/dissort_frequency",
"overridden": false
}]
}

idk how it ended up here.

It shows in search, but you can't open it
image

@anvilfolk
Copy link
Contributor Author

Thank you for digging into it so much, that's really really helpful!

It sounds like generating the cache for the first time causes the freeze. It may or may not be related to there being shaders, or to the fact that shader documentation may be considered "script docs". I'll look into it, and address the other stuff too!

@anvilfolk
Copy link
Contributor Author

anvilfolk commented Sep 24, 2024

I've been wracking my brain on how this race condition could happen but I'm not getting anywhere. @KoBeWi were you perhaps running a -use_asan=true use_ubsan=true build or with --verbose? Both of those things have, in my experience, very significantly increased the time it takes to regenerate the cache. But still nowhere near 8 minutes 🤔 If it hangs consistently, would you be able to run it with --verbose? The doc cache regen should attempt to load every single script.

I could see EditorHelp::get_doc_data() locking only if ResourceLoader::load(p_dir->get_file_path(i));, which is called from EditorHelp::worker_thread in EditorHelp::_reload_scripts_documentation(), hangs from interacting with some other thread also loading stuff. Could that be a possibility?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 24, 2024

I'm using dev_build=yes scu_build=yes fast_unsafe=yes. The last file printed by verbose is an image, preceded by a scene.

I can send you the project if you can't figure it out.

@anvilfolk
Copy link
Contributor Author

That project would help - I could at least inspect where the threads are and figure out what's causing the deadlock.

Turns out shaders are considered scripts for doc purposes and will have their documentation cached when they are compiled, e.g. when a scene with a shader is loaded in the editor. Might need to look into how to remove those docs if the shader is ever deleted.

editor/editor_help.h Outdated Show resolved Hide resolved
@anvilfolk
Copy link
Contributor Author

Latest update:

  1. Changed _script_docs_loaded to SafeFlag
  2. Removed race condition in reading/writing to docs_to_add/remove/etc by pulling setting _script_docs_loaded to before those are read. In that case, other threads will wait until this thread is finished to add those docs.

I noticed that filesystem_changed or sources_changed aren't always emitted. @Hilderin, I'm thinking it may be safer to emit a new first_scan_complete signal to make sure. What are your thoughts?

@anvilfolk
Copy link
Contributor Author

anvilfolk commented Sep 27, 2024

New update:

  • Fixed reason for deadlock found by @KoBeWi, thanks for sharing the project so I could replicate it
  • Changed filesystem_changed which isn't always fired, to sources_changed which should always be fired.

The reason for the deadlock was that non-main threads like the EditorHelp worker thread will queue resource load tasks and wait for them to be dispatched (I assume) on the main thread and loaded from Godot's general worker threads. But, if the main thread waits on the EditorHelp worker thread by calling e.g. EditorHelp::get_doc_data() before dispatching the load tasks, then it deadlocks.

I added a verbose error message to catch when deadlocks might happen. There's a chance more of them happen in other projects with different load pathways that end up calling EditorHelp::get_doc_data(). When I get more time I could go around and replace usages of EditorHelp::get_doc_data() with the new helper methods :)

Also, force-reloading all the scripts from a worker thread in a larger project like @KoBeWi's has resulted in Invalid task ID. a few times @RandomShaper.

@anvilfolk
Copy link
Contributor Author

I'm going to create a new worker thread just for the ResourceLoader::load() calls. It should prevent all deadlocks without having to change all existing calls to EditorHelp::get_doc_data() to use the new helper functions.

@anvilfolk
Copy link
Contributor Author

Latest update:

  • Added a loader_thread to EditorHelp. It is used to make the ResourceLoader::load() calls, which were deadlocking when the main thread wait for the worker_thread. There should be no more deadlocking possible at all.
  • Changed EditorHelpBit and ScriptEditorPlugin to use new EditorHelp doc access methods. Can be replicated throughout the rest of the codebase where appropriate, but it's not necessary.
  • Will run more comprehensive tests when I get back to the right computer

@magian1127
Copy link
Contributor

EDIT2: I saved the cache as .tres and inspected it. It seems to also contain shaders?

It shows in search, but you can't open it !

@KoBeWi

This was caused by my previous PR, and I've now submitted a fix. #97616

@anvilfolk
Copy link
Contributor Author

anvilfolk commented Sep 30, 2024

Testing is showing docs are not when changed when Godot is closed, or when changed from outside Godot with it open. Looking into it. Could be either because something changed in EditorFileSystem which stopped _update_scripts_documentation() from being called (maybe #95678? I'll try reverting to before that), or from my changing from filesystem_changed to sources_changed.

Or, you know, some other skill issue of mine 🤣

This PR adds a script documentation cache in the project folder.
It is loaded at alongside native documentation caches. This makes
scripts fully accessible through Search Help, including their
members, etc, right from project start, without having to compile
every single script.

Co-authored-by: Hilderin <81109165+Hilderin@users.noreply.github.com>
void EditorHelp::regenerate_script_doc_cache() {
if (EditorFileSystem::get_singleton()->is_scanning()) {
// Wait until EditorFileSystem scanning is complete to use updated filesystem structure.
EditorFileSystem::get_singleton()->connect(SNAME("sources_changed"), callable_mp_static(_regenerate_script_doc_cache), CONNECT_ONE_SHOT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to sources_changed from filesystem_changed. It looks like that might be a more reliable signal, since in some situations where no change has happened, filesystem_changed may not emit. I also like that OS::get_singleton()->benchmark_end_measure("Editor", "First Scan"); happens in EditorNode::_sources_changed, which is good evidence that it's reliable to detect the end of the first scan.

ERR_FAIL_COND_MSG(!ProjectSettings::get_singleton()->is_project_loaded(), "Error: cannot load script doc cache without a project.");
ERR_FAIL_COND_MSG(!ResourceLoader::exists(get_script_doc_cache_full_path()), "Error: cannot load script doc cache from inexistent file.");

Ref<Resource> script_doc_cache_res = ResourceLoader::load(get_script_doc_cache_full_path(), "", ResourceFormatLoader::CACHE_MODE_IGNORE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added CACHE_MODE_IGNORE here since it feels silly to cache something we're deleting anyway :)

Perhaps this is worth doing for the native docs as well?

@anvilfolk
Copy link
Contributor Author

anvilfolk commented Oct 7, 2024

Ran the following tests.

With Godot open:

Add script Modify script Remove script
Internal script editor
External script editor

With Godot closed:

Regenerate docs Added script Modified script Removed script
Empty project, no extra scripts
Empty project, extra scripts
Complex project, no extra scripts ⚠️
Complex project, extra scripts ⚠️

The extra scripts are 4096 extra scripts that exist just to make script regen take longer and hopefully bring concurrency issues to light. None did.

Notes:

  • The ⚠️ warnings ⚠️ above refer to getting the occasional Invalid task ID error, presumably because loader_thread is loading gdscript files that may be used & being loaded elsewhere. It is more of a concurrent resource loading problem than something to do with this PR, I think.
  • I also ran into an issue where Godot would straight up close repeatedly near startup with the complex project, but never within VS. It stopped happening, and I couldn't get a log or replicate at all after a while. I think one time before Godot exited, the docs hadn't loaded, so there might be an edge case here somewhere.
  • I'm also getting ObjectDB leaked at exit, but I don't really see how that might be from this. Unless it's something to do with saving a resource on exit? I'm not sure.

Again, bit thanks to @KoBeWi who provided a larger project to bugfix & test on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants