GDScript: Don't clean up other scripts #114801
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #114651
Fixes #114420
Fixes #98141 (tested using the 4.3 MRP by myself. Not the original one.)
Might be related to: #95428 -> No MRP, but my hypothesis is, that script members were cleared from a still referenced script.
#114651 tries to fix the logic which cleans up dependencies. This PR in contrast removes the logic completely.
This means that GDScripts with cyclic dependencies will only be cleaned up at engine shutdown.
The approach is fine, because it's very unlikely for the dependency cleanup logic to run at all, because it is only called from the GDScript destructor. The
GDScriptCacheholds refs to all GDScripts so this destructor never runs until engine shutdown. The only exception are inner classes but they are referenced by their outer class. So for them to get freed an outer script needs to drop a ref to an inner class. This mostly happens in editor when editing scripts.More verbose rubberducking
This whole cycle detection is located in
GDScript::clear. There are three possible calls toclearGDScriptLanguage::finalize-> irrelevant, the whole island detection is protected to not run during language shutdownGDScript::clear-> recursion, skip~GDScriptSo for this to run the destructor of a GDScript needs to run. GDScript's are ref counted so the ref count of a GDScript needs to go to 0.
GDScriptCache::full_gdscript_cacheholds a reference to every GDScript.(only exception are inner classes those are not referenced by the GDScript cache. But they are referenced by their outer GDScripts).
So for the refcount to drop to to 0 one of those things needs to happen:
A. The
GDScriptCachedrops it reference to a gdscriptB. An outer GDScript drops it reference to an inner gdscript
About
A: this is very unlikely. The only easy way isGDScriptCache::remove_scriptwhich only gets called fromGDScript::clearand some unused method. Some edge cases withtake_over_pathmight also be able to trigger it I guess.B: This can only really happen if the outer script gets destructed (see
A). It also happens when editing scripts in the script editor. And maybe when messing withreloadI guess.And well you can force it with
RefCounted::unreference.All those possibilities seem so far fetched. So why even bother if all we can safe is some memory from the deps of inner classes. Which will be basically nothing because inner scripts will share most of their deps with their outer class anyway.