Skip to content

Commit

Permalink
Add a Reference Cycle Breaker
Browse files Browse the repository at this point in the history
As you may know Godot does not use a garbage collector and thanks to that
there are no collection stalls. Instead, it uses reference counting.

While Godot resoucres are designed so its extremely hard to have reference cycles,
it is possible on the script side to have a reference cycle and thus leak
script resources. Godot will report these leaked objects on exit, but sometimes
it can be hard to find and track them down.

This PR adds a cycle breaker. It is used like this:

```GDScript
get_tree().break_reference_cycles()
```

The cycle breaker basically tracks and tags anything connected to the scene
tree and engine singletons. It does this very quickly, so you can call this
function during loading screen, game exit or pauses.

It has a few modes in which it functions:

* Script only: This only tracks cycles in script classes not connected to the scene tree and singletons and breaks them. This mode should be safe to use.
* All objects: This tracks any object not connected to the scene tree and singletons and clears all resources properties on them to clear cycles. This is not advisable to use unless you know what you are doing.

Additionally, the type of tagging can be customized:
* Only scan resources, do not get into containers: This is the fastest mode, but if you have scripts referenced from containers (array or dictionary) it will miss them.
* Containers: This mode will go into containers. It is more effective, but slower.

Finally, there is the option to print what was collected. This is intended in case you want to debug a leak and resolve it instead of relying on the cycle breaker.

This function returns how many cycles were broken, which may add in debugging and CI tasks.

TODO:

- [ ] I think some GDScript globals and static variables are not being tracked, but need to ask the GDScript team about this.
- [ ] Some singletons or global objects may not be tracked or may be too hard to track. Possibly may be good to simply add them to an ignore list (via some virtual function to ignore them on tagging).
- [ ] Maybe warn the user if there are threads running (likely via WorkerThreadPool query). While the ObjectID access is thread safe, this function shoud ideally be run after joining all threads.

Feedback welcome!
  • Loading branch information
reduz authored and dreed-sd committed Apr 26, 2024
1 parent 01ba66f commit e4589b2
Show file tree
Hide file tree
Showing 22 changed files with 355 additions and 2 deletions.
51 changes: 50 additions & 1 deletion core/object/class_db.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1091,6 +1091,14 @@ void ClassDB::add_property(const StringName &p_class, const PropertyInfo &p_pinf
OBJTYPE_WLOCK

type->property_list.push_back(p_pinfo);
if (p_pinfo.type == Variant::OBJECT) {
type->resource_properties.push_back(p_pinfo.name);
} else if (p_pinfo.type == Variant::ARRAY) {
type->array_properties.push_back(p_pinfo.name);
} else if (p_pinfo.type == Variant::DICTIONARY) {
type->dict_properties.push_back(p_pinfo.name);
}

type->property_map[p_pinfo.name] = p_pinfo;
#ifdef DEBUG_METHODS_ENABLED
if (mb_get) {
Expand Down Expand Up @@ -1852,4 +1860,45 @@ void ClassDB::cleanup() {
native_structs.clear();
}

//
void ClassDB::tag_collect_pass(Object *p_object, uint32_t p_pass, bool p_containers) {
ClassInfo *check = classes.getptr(p_object->get_class_name());
while (check) {
for (uint32_t i = 0; i < check->resource_properties.size(); i++) {
Ref<Resource> res = p_object->get(check->resource_properties[i]);
if (res.is_valid()) {
res->tag_collect_pass(p_pass, p_containers);
}
}
if (p_containers) {
for (uint32_t i = 0; i < check->dict_properties.size(); i++) {
Dictionary dict = p_object->get(check->dict_properties[i]);
dict.tag_collect_pass(p_pass, p_containers);
}

for (uint32_t i = 0; i < check->array_properties.size(); i++) {
Array arr = p_object->get(check->array_properties[i]);
arr.tag_collect_pass(p_pass, p_containers);
}
}
check = check->inherits_ptr;
}
}

void ClassDB::unlink_resources(Object *p_object, bool p_containers) {
ClassInfo *check = classes.getptr(p_object->get_class_name());
while (check) {
for (uint32_t i = 0; i < check->resource_properties.size(); i++) {
p_object->set(check->resource_properties[i], Ref<Resource>());
}
if (p_containers) {
for (uint32_t i = 0; i < check->dict_properties.size(); i++) {
p_object->set(check->dict_properties[i], Dictionary());
}

for (uint32_t i = 0; i < check->array_properties.size(); i++) {
p_object->set(check->array_properties[i], Array());
}
}
check = check->inherits_ptr;
}
}
9 changes: 9 additions & 0 deletions core/object/class_db.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,12 @@ class ClassDB {
HashMap<StringName, MethodInfo> signal_map;
List<PropertyInfo> property_list;
HashMap<StringName, PropertyInfo> property_map;

// For tagging
LocalVector<StringName> resource_properties;
LocalVector<StringName> array_properties;
LocalVector<StringName> dict_properties;

#ifdef DEBUG_METHODS_ENABLED
List<StringName> constant_order;
List<StringName> method_order;
Expand Down Expand Up @@ -450,6 +456,9 @@ class ClassDB {
static void get_native_struct_list(List<StringName> *r_names);
static String get_native_struct_code(const StringName &p_name);
static uint64_t get_native_struct_size(const StringName &p_name); // Used for asserting

static void tag_collect_pass(Object *object, uint32_t p_pass, bool p_containers);
static void unlink_resources(Object *p_object, bool p_containers);
};

#define BIND_ENUM_CONSTANT(m_constant) \
Expand Down
52 changes: 51 additions & 1 deletion core/object/object.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -857,20 +857,25 @@ void Object::notification(int p_notification, bool p_reversed) {
}

String Object::to_string() {
String id = itos(get_instance_id());
if (script_instance) {
bool valid;
String ret = script_instance->to_string(&valid);
if (valid) {
return ret;
}
if (script_instance->get_script().is_valid()) {
id = script_instance->get_script()->get_script_name();
}
}

if (_extension && _extension->to_string) {
String ret;
GDExtensionBool is_valid;
_extension->to_string(_extension_instance, &is_valid, &ret);
return ret;
}
return "<" + get_class() + "#" + itos(get_instance_id()) + ">";
return "<" + get_class() + "#" + id + ">";
}

void Object::set_script_and_instance(const Variant &p_script, ScriptInstance *p_instance) {
Expand Down Expand Up @@ -2068,6 +2073,34 @@ void ObjectDB::debug_objects(DebugFunc p_func) {
spin_lock.unlock();
}

void Object::tag_collect_pass(uint32_t p_pass, bool p_collect_containers) {
if (collect_pass == p_pass) {
return; // Already collected.
}

collect_pass = p_pass; // Mark beforehand due to recursion.

if (script_instance) {
script_instance->tag_collect_pass(p_pass, p_collect_containers);
Ref<Script> scr = script;
if (scr.is_valid()) {
scr->tag_collect_pass(p_pass, p_collect_containers);
}
}

ClassDB::tag_collect_pass(this, p_pass, p_collect_containers);

_tag_collect_pass_customv(p_pass, p_collect_containers);

for (const KeyValue<StringName, Variant> &K : metadata) {
K.value.tag_collect_pass(p_pass, p_collect_containers);
}
}

void Object::unlink_resources(bool p_collect_containers) {
ClassDB::unlink_resources(this, p_collect_containers);
}

void Object::get_argument_options(const StringName &p_function, int p_idx, List<String> *r_options) const {
if (p_idx == 0) {
if (p_function == "connect" || p_function == "is_connected" || p_function == "disconnect" || p_function == "emit_signal" || p_function == "has_signal") {
Expand Down Expand Up @@ -2243,3 +2276,20 @@ void ObjectDB::cleanup() {
memfree(object_slots);
}
}

void ObjectDB::fetch_unliked_objects(uint32_t p_collect_pass, List<ObjectID> &r_unlinked) {
spin_lock.lock();
for (uint32_t i = 0, count = slot_count; i < slot_max && count != 0; i++) {
if (object_slots[i].validator) {
if (object_slots[i].object->get_collect_pass() != p_collect_pass) {
uint64_t id = object_slots[i].validator;
id <<= OBJECTDB_SLOT_MAX_COUNT_BITS;
id |= uint64_t(i);

r_unlinked.push_back(ObjectID(id));
}
count--;
}
}
spin_lock.unlock();
}
22 changes: 22 additions & 0 deletions core/object/object.h
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,15 @@ protected:
_validate_property(p_property); \
} \
} \
_FORCE_INLINE_ void (Object::*_get_tag_collect_pass_custom() const)(uint32_t p_pass, bool p_containers) const { \
return (void(Object::*)(uint32_t, bool) const) & m_class::_tag_collect_pass_custom; \
} \
virtual void _tag_collect_pass_customv(uint32_t p_pass, bool p_containers) const override { \
m_inherits::_tag_collect_pass_customv(p_pass, p_containers); \
if (m_class::_get_tag_collect_pass_custom() != m_inherits::_get_tag_collect_pass_custom()) { \
_tag_collect_pass_custom(p_pass, p_containers); \
} \
} \
_FORCE_INLINE_ bool (Object::*_get_property_can_revert() const)(const StringName &p_name) const { \
return (bool(Object::*)(const StringName &) const) & m_class::_property_can_revert; \
} \
Expand Down Expand Up @@ -634,6 +643,7 @@ class Object {
uint32_t _edited_version = 0;
HashSet<String> editor_section_folding;
#endif
uint32_t collect_pass = 0;
ScriptInstance *script_instance = nullptr;
Variant script; // Reference does not exist yet, store it in a Variant.
HashMap<StringName, Variant> metadata;
Expand Down Expand Up @@ -693,6 +703,7 @@ class Object {
virtual bool _getv(const StringName &p_name, Variant &r_property) const { return false; };
virtual void _get_property_listv(List<PropertyInfo> *p_list, bool p_reversed) const {};
virtual void _validate_propertyv(PropertyInfo &p_property) const {};
virtual void _tag_collect_pass_customv(uint32_t p_pass, bool p_containers) const {};
virtual bool _property_can_revertv(const StringName &p_name) const { return false; };
virtual bool _property_get_revertv(const StringName &p_name, Variant &r_property) const { return false; };
virtual void _notificationv(int p_notification, bool p_reversed) {}
Expand All @@ -703,6 +714,7 @@ class Object {
bool _get(const StringName &p_name, Variant &r_property) const { return false; };
void _get_property_list(List<PropertyInfo> *p_list) const {};
void _validate_property(PropertyInfo &p_property) const {};
void _tag_collect_pass_custom(uint32_t p_pass, bool p_containers) const {};
bool _property_can_revert(const StringName &p_name) const { return false; };
bool _property_get_revert(const StringName &p_name, Variant &r_property) const { return false; };
void _notification(int p_notification) {}
Expand All @@ -725,6 +737,9 @@ class Object {
_FORCE_INLINE_ void (Object::*_get_validate_property() const)(PropertyInfo &p_property) const {
return &Object::_validate_property;
}
_FORCE_INLINE_ void (Object::*_get_tag_collect_pass_custom() const)(uint32_t p_pass, bool p_containers) const {
return &Object::_tag_collect_pass_custom;
}
_FORCE_INLINE_ bool (Object::*_get_property_can_revert() const)(const StringName &p_name) const {
return &Object::_property_can_revert;
}
Expand Down Expand Up @@ -985,6 +1000,10 @@ class Object {

void cancel_free();

void tag_collect_pass(uint32_t p_pass, bool p_collect_containers = false);
_FORCE_INLINE_ uint32_t get_collect_pass() const { return collect_pass; }
virtual void unlink_resources(bool p_collect_containers = false);

Object();
virtual ~Object();
};
Expand Down Expand Up @@ -1047,6 +1066,9 @@ class ObjectDB {

return object;
}

static void fetch_unliked_objects(uint32_t p_collect_pass, List<ObjectID> &r_unlinked);

static void debug_objects(DebugFunc p_func);
static int get_object_count();
};
Expand Down
2 changes: 2 additions & 0 deletions core/object/script_instance.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ class ScriptInstance {

virtual const Variant get_rpc_config() const;

virtual void tag_collect_pass(uint32_t p_pass, bool p_collect_containers = false) {}

virtual ScriptLanguage *get_language() = 0;
virtual ~ScriptInstance();
};
Expand Down
24 changes: 24 additions & 0 deletions core/object/script_language.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,17 @@ bool ScriptServer::scripting_enabled = true;
bool ScriptServer::reload_scripts_on_save = false;
ScriptEditRequestFunction ScriptServer::edit_request_func = nullptr;

String Script::get_script_name() const {
if (get_global_name() != StringName()) {
return get_global_name();
} else if (!get_path().is_empty()) {
return get_path().get_file();
} else if (!get_name().is_empty()) {
return get_name(); // Resource name.
}
return get_class();
}

void Script::_notification(int p_what) {
switch (p_what) {
case NOTIFICATION_POSTINITIALIZE: {
Expand Down Expand Up @@ -747,6 +758,19 @@ Variant PlaceHolderScriptInstance::property_get_fallback(const StringName &p_nam
return Variant();
}

void PlaceHolderScriptInstance::tag_collect_pass(uint32_t p_pass, bool p_collect_containers) {
if (collect_pass == p_pass) {
return;
}

collect_pass = p_pass; // Tag beforehand due to recursion.

for (const KeyValue<StringName, Variant> &E : values) {
const Variant &v = E.value;
v.tag_collect_pass(p_pass, p_collect_containers);
}
}

PlaceHolderScriptInstance::PlaceHolderScriptInstance(ScriptLanguage *p_language, Ref<Script> p_script, Object *p_owner) :
owner(p_owner),
language(p_language),
Expand Down
4 changes: 4 additions & 0 deletions core/object/script_language.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ class Script : public Resource {
public:
virtual bool can_instantiate() const = 0;

virtual String get_script_name() const;
virtual Ref<Script> get_base_script() const = 0; //for script inheritance
virtual StringName get_global_name() const = 0;
virtual bool inherits_script(const Ref<Script> &p_script) const = 0;
Expand Down Expand Up @@ -409,6 +410,7 @@ class PlaceHolderScriptInstance : public ScriptInstance {
HashMap<StringName, Variant> constants;
ScriptLanguage *language = nullptr;
Ref<Script> script;
uint32_t collect_pass = 0;

public:
virtual bool set(const StringName &p_name, const Variant &p_value) override;
Expand Down Expand Up @@ -444,6 +446,8 @@ class PlaceHolderScriptInstance : public ScriptInstance {

virtual const Variant get_rpc_config() const override { return Variant(); }

virtual void tag_collect_pass(uint32_t p_pass, bool p_collect_containers = false) override;

PlaceHolderScriptInstance(ScriptLanguage *p_language, Ref<Script> p_script, Object *p_owner);
~PlaceHolderScriptInstance();
};
Expand Down
20 changes: 20 additions & 0 deletions core/variant/array.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,28 @@ class ArrayPrivate {
Vector<Variant> array;
Variant *read_only = nullptr; // If enabled, a pointer is used to a temporary value that is used to return read-only values.
ContainerTypeValidate typed;
uint32_t collect_pass = 0;
};

void Array::tag_collect_pass(uint32_t p_pass, bool p_collect_containers) const {
if (_p->collect_pass == p_pass) {
return;
}

_p->collect_pass = p_pass; // Mark beforehand due to recursion.

if (_p->typed.type != Variant::OBJECT && _p->typed.type != Variant::DICTIONARY && _p->typed.type != Variant::ARRAY && _p->typed.type != Variant::NIL) {
// None of these types will contain objects, so do not do anything here.
return;
}

int len = _p->array.size();
const Variant *arr = _p->array.ptr();
for (int i = 0; i < len; i++) {
arr[i].tag_collect_pass(p_pass, p_collect_containers);
}
}

void Array::_ref(const Array &p_from) const {
ArrayPrivate *_fp = p_from._p;

Expand Down
2 changes: 2 additions & 0 deletions core/variant/array.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ class Array {
void make_read_only();
bool is_read_only() const;

void tag_collect_pass(uint32_t p_pass, bool p_collect_containers = false) const;

Array(const Array &p_base, uint32_t p_type, const StringName &p_class_name, const Variant &p_script);
Array(const Array &p_from);
Array();
Expand Down
18 changes: 18 additions & 0 deletions core/variant/dictionary.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,26 @@ struct DictionaryPrivate {
SafeRefCount refcount;
Variant *read_only = nullptr; // If enabled, a pointer is used to a temporary value that is used to return read-only values.
HashMap<Variant, Variant, VariantHasher, StringLikeVariantComparator> variant_map;
uint32_t collect_pass = 0;
};

void Dictionary::tag_collect_pass(uint32_t p_pass, bool p_collect_containers) const {
if (_p->collect_pass == p_pass) {
return;
}

/* Add typed dictionary check here eventually */

_p->collect_pass = p_pass; // Mark beforehand due to recursion.

for (const KeyValue<Variant, Variant> &E : _p->variant_map) {
for (int i = 0; i < 2; i++) {
const Variant &v = i == 0 ? E.key : E.value;
v.tag_collect_pass(p_pass, p_collect_containers);
}
}
}

void Dictionary::get_key_list(List<Variant> *p_keys) const {
if (_p->variant_map.is_empty()) {
return;
Expand Down
2 changes: 2 additions & 0 deletions core/variant/dictionary.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ class Dictionary {

const void *id() const;

void tag_collect_pass(uint32_t p_pass, bool p_collect_containers) const;

Dictionary(const Dictionary &p_from);
Dictionary();
~Dictionary();
Expand Down
7 changes: 7 additions & 0 deletions core/variant/variant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3708,6 +3708,13 @@ String Variant::get_callable_error_text(const Callable &p_callable, const Varian
}
}

void Variant::_tag_collect_pass_object(uint32_t p_pass, bool p_collect_containers) const {
Object *instance = ObjectDB::get_instance(_get_obj().id);
if (instance) {
instance->tag_collect_pass(p_pass, p_collect_containers);
}
}

void Variant::register_types() {
_register_variant_operators();
_register_variant_methods();
Expand Down
Loading

0 comments on commit e4589b2

Please sign in to comment.