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

Fix loading scene for every request on script file for workspace completion #101091

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dekaravanhoc
Copy link

For workspace completion the Scene for the script is loaded for every completion request and freed afterwards. Resulting in performance issues for scenes including large resources.

Loaded Scenes are now cached for each script. Not loading them again and only freeing them when the connection is terminated or the language server stopped.

This PR fixes #100934


I have no experience in C++ development - feedback is of course more than welcome.
Unit Tests are green and auto formatting should have worked too.

@dekaravanhoc dekaravanhoc requested a review from a team as a code owner January 3, 2025 19:59
@dekaravanhoc dekaravanhoc force-pushed the lsp/fix_performance_scene_loading_workspace_completion branch from 186fd24 to c2ac0a5 Compare January 3, 2025 20:30
@HolonProduction
Copy link
Member

This should remove the delay for subsequent completion requests, the delay would still be there for the initial request. I think we could improve on this by using the textDocument/didOpen and textDocument/didClose signals to manage the cache. This way Godot could start loading the owner, when the script gets opened. Depending on the time between opening and typing something (which can be quite a bit in bigger projects from my experience) this might completely remove the delay or decrease it a bit.

Also Godot resource loading system has a builtin cache which would definitely come in handy with the preloading approach. As long as the loaded resource isn't freed it will be reused in the future. We could just cache the references to the loaded scene and re instantiate it since this shouldn't cause much overhead (this would also allow replacing SceneCache with a normal HashMap).

This could work kinda like this:

  • in didOpen:
    • use ResourceLoader.load_threaded_request to start loading the owner
    • add an invalid Ref to the hashmap for the path (we need to keep track of this in case the user opens files but does never complete anything)
  • in completion:
    • use ResourceLoader.load_threaded_request again to make sure a loading process is going on for completions after the first one
    • obtain the resource with ResourceLoader.load_threaded_get (blocking like before if the resource isn't fully loaded yet)
    • then safe the resource into the hash map under the path (this will keep the resource in memory thus automatically using the resource loader cache in the future)
  • in didClose (and on disconnect for external editor crashes):
    • remove entry from hash map and if entry was invalid use ResourceLoader.load_threaded_get to prevent leaking results

@dekaravanhoc
Copy link
Author

Hey thanks,

the SceneCache is only a wrapper for a HashMap with a Scene specific clear() function.

Also as these Scenes are loaded resources and their sub resources also, the ResourceLoader already uses the cache for all of them, if there are used in another loaded Scene.

For the rest, sounds reasonable. I'll look into it. Also I have really fast completion times even at the beginning now with a production build and large resources (~1GB loaded), as the main slowdown came from the queue of loading->freeing->loading->freeing.

@HolonProduction
Copy link
Member

Also as these Scenes are loaded resources and their sub resources also, the ResourceLoader already uses the cache for all of them, if there are used in another loaded Scene.

Yeah right, my main reason for suggesting only relying on the resource loader cache was the idea with the loading headstart, which might get harder to implement when caching the nodes.

For the rest, sounds reasonable. I'll look into it. Also I have really fast completion times even at the beginning now with a production build and large resources (~1GB loaded), as the main slowdown came from the queue of loading->freeing->loading->freeing.

That's interesting, I still get the same slow down on the first completion request, with this PR and my MRP (no production build). Maybe we are hitting different bottle necks with loading 🤔

@dekaravanhoc
Copy link
Author

That's interesting, I still get the same slow down on the first completion request, with this PR and my MRP (no production build). Maybe we are hitting different bottle necks with loading 🤔

Ah k. I used a production build (ucrt). On my dev build it was in deed still very slow, even for the first input. But I did not stop time, so could just be my imagination over being happy that it worked. 🙃
That said, I think, its a good idea to load the scene when opening the script.

@AThousandShips AThousandShips added this to the 4.x milestone Jan 4, 2025
Dekara VanHoc added 2 commits January 6, 2025 13:25
…letion

For workspace completion the Scene for the script is loaded for every
completion request and freed afterwards. Resulting in performance issues
for scenes including large resources.

Loaded Scenes are now cached for each script and loaded when opening the
script. Not loading them again and only freeing them when the connection
is terminated, the language server stopped or the script is closed.
If use thread is enabled in the language server, threaded load from
ResourceLoader will be utilized.

Only one load task will be running at a time.
@dekaravanhoc dekaravanhoc force-pushed the lsp/fix_performance_scene_loading_workspace_completion branch from c2ac0a5 to 44565a1 Compare January 6, 2025 12:41
@dekaravanhoc
Copy link
Author

I moved all logic for the Scene loading into its own class SceneCache inside language_server_protocol.

Threaded resource Loading is added and used, when "use thread" is activated for the language server.
Its quite nice, as the loading now does not block the server for auto completion etc. And as far as I can see, the Scene is only used for Node path completion - definitively a win on scenes with large resources.

Using threaded resource loading is a bit of a hassle, as there is no option to kill a specific running task. (Or at least I did not found one.) So the server needs to wait for a running task and clean it afterwards, if the client disconnects or closes the document.
Also running multiple tasks in parallel that might load the same sub resources can bring up errors in the editor, so I opted for one task at a time.

But for now, I think it works quite well.

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.

LSP performance issues on "slower" devices
3 participants