Speed up GDScript::get_must_clear_dependencies()

[eldidou]

get_must_clear_dependencies() has a N^3log(N) time complexity, and this can very quickly slow down the quitting process as more gdscripts are added in a project. This change improves it to N^2log(N).
Instead of using all the inverted dependencies, we do the same with all (non-inverted) dependencies, which is N times faster.

Fixes #85435

[akien-mga]

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

[eldidou]

Sorry about that (I'm quite new to this process), now it should be linked to the correct account.

[kleonc]

get_must_clear_dependencies() has a N^3_log(N) time complexity, and this can very quickly slow down the quitting process as more gdscripts are added in a project. This change improves it to N^2_log(N).

This is an improvement indeed but I think GDScriptLanguage::finish() shouldn't lead to any checks for dependencies like that? 🤔 On finalization all scripts are to-be-cleared anyway, so there's no point to check/clear the full chain of dependencies for every script. Iterating over every script and clearing only its own refs to any other script should be sufficient to get rid of all cross dependencies. Meaning the finalization should be doable linearly (O(N)).

Note that I'm not really familiar with the GDScript internals so maybe I'm missing something and it's not that simple?

Anyway, could potentially be done in another PR, this is already an improvement.

[adamscott]

This is an improvement indeed but I think GDScriptLanguage::finish() shouldn't lead to any checks for dependencies like that?

But this isn't about GDScriptLanguage::finish(), isn't it?

[RevoluPowered]

Can confirm that with The Mirror we see a hugely expensive shutdown time because of this function.

Benchmarked using superluminal profiler and it showed this function was consuming 100% CPU for over 90 seconds during shutdown.

I'll test this fix and report back the app close time :)

[kleonc]

But this isn't about GDScriptLanguage::finish(), isn't it?

It is and it isn't, depends how you look at it. 🙃

#85435 which this PR aims to fix is specifically about slow quiting (both editor/runtime) when there are many cross referencing/dependant scripts. This is caused by the slow execution of GDScriptLanguage::finish(). And why is GDScriptLanguage::finish() so slow? Because it's calling GDScript:clear() many times and GDScript::clear() is slow with the current implementation.

So this PR makes GDScript:clear() faster which is good on its own, regardless of what calls GDScript::clear(). 👍 This of course makes also GDScriptLanguage::finish() faster as a result, which improves the quitting time.

What I've said in #85603 (comment) is that GDScriptLanguage::finish() can potentially be made faster as it shouldn't need to call GDScript:clear() in its current form at all, because on finalizing all scripts clearing non-direct script dependencies shouldn't be needed (and that's what's done as a part of GDScript:clear()). Clearing only direct dependencies for all scripts would also result in no cross dependencies in the end and would be faster.

TLDR: this PR is good on its own but GDScriptLanguage::finish() could still be made faster.

---

BTW Yeah, would be nice if some benchmark times would be provided for before/after. In the end even if it helps with #85435 (it does) it still might need be considered as not fixing it if the timings are still not satisfying.

[adamscott]

@eldidou Thanks for your PR! For a first PR, that's a nice one!

I'm trying to see if there's things that could break with your implementation, but the logic seems fine. The idea to skip the dependencies if they don't include the current script is a good idea. It does what I thought to do when I implemented inverse dependency checks.

I asked @vnen and @dalexeev to review it, but so far, I like it.

[RevoluPowered]

Can confirm that with The Mirror we see a hugely expensive shutdown time because of this function.

Benchmarked using superluminal profiler and it showed this function was consuming 100% CPU for over 90 seconds during shutdown.

I'll test this fix and report back the app close time :)

4 seconds to close the editor (previously multiple minutes)
4 seconds to close the app (previously 2 minutes or more)

This makes it much more deterministic for me.

Tested on windows.

[adamscott]

Seems fine to me. It improved my logic and I don't see how the new logic would not cover my old one.

Especially with all the tests passing, I'm OK merging this very soon.

[YuriSizov]

Thanks! And congratulations on your first merged Godot contribution! That's a very cool way to enter the space 🙂

[eldidou]

Thanks! Glad to help 🙂

[adamscott]

@eldidou Don't hesitate to join our developer's chat. We have a GDScript public meeting tomorrow.

[YuriSizov]

Cherry-picked for 4.2.2.

[mihe]

Seeing as how this slowdown still remains a problem when you go into the hundreds of scripts (with more complex dependencies), I've gone ahead and made the change that was talked about earlier in this thread, by simply skipping the must_clear_dependencies work altogether when in GDScriptLanguage::finish. You'll find the change in #94505.

It seemed like a straight-forward enough of a change, and passes all the unit tests and whatnot, but I'd appreciate some more eyes on it.