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.

Sign up for GitHub

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 lambda cross-thread dynamics (take 2) #85248

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Fix lambda cross-thread dynamics (take 2) #85248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f26328e
Revert recently added approach to cross-thread lambda survival
RandomShaper Nov 22, 2023
bfe66ab
Fixup thread-owned lambda bookkeeping on thread exit (take 2)
RandomShaper Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions core/templates/list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ class List {
data->erase(this);
}

void transfer_to_back(List<T, A> *p_dst_list);

_FORCE_INLINE_ Element() {}
};

Expand Down Expand Up @@ -762,4 +764,41 @@ class List {
}
};

template <class T, class A>
void List<T, A>::Element::transfer_to_back(List<T, A> *p_dst_list) {
// Detach from current.

if (data->first == this) {
data->first = data->first->next_ptr;
}
if (data->last == this) {
data->last = data->last->prev_ptr;
}
if (prev_ptr) {
prev_ptr->next_ptr = next_ptr;
}
if (next_ptr) {
next_ptr->prev_ptr = prev_ptr;
}
data->size_cache--;

// Attach to the back of the new one.

if (!p_dst_list->_data) {
p_dst_list->_data = memnew_allocator(_Data, A);
p_dst_list->_data->first = this;
p_dst_list->_data->last = nullptr;
p_dst_list->_data->size_cache = 0;
prev_ptr = nullptr;
} else {
p_dst_list->_data->last->next_ptr = this;
prev_ptr = p_dst_list->_data->last;
}
p_dst_list->_data->last = this;
next_ptr = nullptr;

data = p_dst_list->_data;
p_dst_list->_data->size_cache++;
}

#endif // LIST_H
149 changes: 65 additions & 84 deletions modules/gdscript/gdscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,108 +1391,51 @@ String GDScript::debug_get_script_name(const Ref<Script> &p_script) {
}
#endif

thread_local GDScript::UpdatableFuncPtr GDScript::func_ptrs_to_update_thread_local;
GDScript::UpdatableFuncPtr *GDScript::func_ptrs_to_update_main_thread = &func_ptrs_to_update_thread_local;
GDScript::UpdatableFuncPtr GDScript::func_ptrs_to_update_main_thread;
thread_local GDScript::UpdatableFuncPtr *GDScript::func_ptrs_to_update_thread_local = nullptr;

List<GDScript::UpdatableFuncPtrElement>::Element *GDScript::_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr) {
MutexLock lock(func_ptrs_to_update_mutex);

List<UpdatableFuncPtrElement>::Element *result = func_ptrs_to_update_elems.push_back(UpdatableFuncPtrElement());
GDScript::UpdatableFuncPtrElement GDScript::_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr) {
UpdatableFuncPtrElement result = {};

{
MutexLock lock2(func_ptrs_to_update_thread_local.mutex);
result->get().element = func_ptrs_to_update_thread_local.ptrs.push_back(p_func_ptr_ptr);
result->get().mutex = &func_ptrs_to_update_thread_local.mutex;
MutexLock lock(func_ptrs_to_update_thread_local->mutex);
result.element = func_ptrs_to_update_thread_local->ptrs.push_back(p_func_ptr_ptr);
result.func_ptr = func_ptrs_to_update_thread_local;

if (likely(func_ptrs_to_update_thread_local.initialized)) {
if (likely(func_ptrs_to_update_thread_local->initialized)) {
return result;
}

func_ptrs_to_update_thread_local.initialized = true;
func_ptrs_to_update_thread_local->initialized = true;
}

func_ptrs_to_update.push_back(&func_ptrs_to_update_thread_local);
MutexLock lock(func_ptrs_to_update_mutex);
func_ptrs_to_update.push_back(func_ptrs_to_update_thread_local);
func_ptrs_to_update_thread_local->rc++;

return result;
}

void GDScript::_remove_func_ptr_to_update(List<UpdatableFuncPtrElement>::Element *p_func_ptr_element) {
// None of these checks should ever fail, unless there's a bug.
// They can be removed once we are sure they never catch anything.
// Left here now due to extra safety needs late in the release cycle.
ERR_FAIL_NULL(p_func_ptr_element);
MutexLock lock(func_ptrs_to_update_thread_local.mutex);
ERR_FAIL_NULL(p_func_ptr_element->get().element);
ERR_FAIL_NULL(p_func_ptr_element->get().mutex);
MutexLock lock2(*p_func_ptr_element->get().mutex);
p_func_ptr_element->get().element->erase();
p_func_ptr_element->erase();
void GDScript::_remove_func_ptr_to_update(const UpdatableFuncPtrElement &p_func_ptr_element) {
ERR_FAIL_NULL(p_func_ptr_element.element);
ERR_FAIL_NULL(p_func_ptr_element.func_ptr);
MutexLock lock(p_func_ptr_element.func_ptr->mutex);
p_func_ptr_element.element->erase();
}

void GDScript::_fixup_thread_function_bookkeeping() {
// Transfer the ownership of these update items to the main thread,
// because the current one is dying, leaving theirs orphan, dangling.

HashSet<GDScript *> scripts;

DEV_ASSERT(!Thread::is_main_thread());
MutexLock lock(func_ptrs_to_update_main_thread->mutex);

{
MutexLock lock2(func_ptrs_to_update_thread_local.mutex);

while (!func_ptrs_to_update_thread_local.ptrs.is_empty()) {
// Transfer the thread-to-script records from the dying thread to the main one.

List<GDScriptFunction **>::Element *E = func_ptrs_to_update_thread_local.ptrs.front();
List<GDScriptFunction **>::Element *new_E = func_ptrs_to_update_main_thread->ptrs.push_front(E->get());

GDScript *script = (*E->get())->get_script();
if (!scripts.has(script)) {
scripts.insert(script);

// Replace dying thread by the main thread in the script-to-thread records.

MutexLock lock3(script->func_ptrs_to_update_mutex);
DEV_ASSERT(script->func_ptrs_to_update.find(&func_ptrs_to_update_thread_local));
{
for (List<UpdatableFuncPtrElement>::Element *F = script->func_ptrs_to_update_elems.front(); F; F = F->next()) {
bool is_dying_thread_entry = F->get().mutex == &func_ptrs_to_update_thread_local.mutex;
if (is_dying_thread_entry) {
// This may lead to multiple main-thread entries, but that's not a problem
// and allows to reuse the element, which is needed, since it's tracked by pointer.
F->get().element = new_E;
F->get().mutex = &func_ptrs_to_update_main_thread->mutex;
}
}
}
}
MutexLock lock(func_ptrs_to_update_main_thread.mutex);
MutexLock lock2(func_ptrs_to_update_thread_local->mutex);

E->erase();
}
}
func_ptrs_to_update_main_thread->initialized = true;

{
// Remove orphan thread-to-script entries from every script.
// FIXME: This involves iterating through every script whenever a thread dies.
// While it's OK that thread creation/destruction are heavy operations,
// additional bookkeeping can be used to outperform this brute-force approach.

GDScriptLanguage *gd_lang = GDScriptLanguage::get_singleton();

MutexLock lock2(gd_lang->mutex);

for (SelfList<GDScript> *s = gd_lang->script_list.first(); s; s = s->next()) {
GDScript *script = s->self();
for (List<UpdatableFuncPtr *>::Element *E = script->func_ptrs_to_update.front(); E; E = E->next()) {
bool is_dying_thread_entry = &E->get()->mutex == &func_ptrs_to_update_thread_local.mutex;
if (is_dying_thread_entry) {
E->erase();
break;
}
}
}
while (!func_ptrs_to_update_thread_local->ptrs.is_empty()) {
List<GDScriptFunction **>::Element *E = func_ptrs_to_update_thread_local->ptrs.front();
E->transfer_to_back(&func_ptrs_to_update_main_thread.ptrs);
func_ptrs_to_update_thread_local->transferred = true;
}
}

Expand All @@ -1516,12 +1459,29 @@ void GDScript::clear(ClearData *p_clear_data) {
{
MutexLock outer_lock(func_ptrs_to_update_mutex);
for (UpdatableFuncPtr *updatable : func_ptrs_to_update) {
MutexLock inner_lock(updatable->mutex);
for (GDScriptFunction **func_ptr_ptr : updatable->ptrs) {
*func_ptr_ptr = nullptr;
bool destroy = false;
{
MutexLock inner_lock(updatable->mutex);
if (updatable->transferred) {
func_ptrs_to_update_main_thread.mutex.lock();
}
for (GDScriptFunction **func_ptr_ptr : updatable->ptrs) {
*func_ptr_ptr = nullptr;
}
DEV_ASSERT(updatable->rc != 0);
updatable->rc--;
if (updatable->rc == 0) {
destroy = true;
}
if (updatable->transferred) {
func_ptrs_to_update_main_thread.mutex.unlock();
}
}
if (destroy) {
DEV_ASSERT(updatable != &func_ptrs_to_update_main_thread);
memdelete(updatable);
}
}
func_ptrs_to_update_elems.clear();
}

RBSet<GDScript *> must_clear_dependencies = get_must_clear_dependencies();
Expand Down Expand Up @@ -2141,8 +2101,25 @@ void GDScriptLanguage::remove_named_global_constant(const StringName &p_name) {
named_globals.erase(p_name);
}

void GDScriptLanguage::thread_enter() {
GDScript::func_ptrs_to_update_thread_local = memnew(GDScript::UpdatableFuncPtr);
}

void GDScriptLanguage::thread_exit() {
GDScript::_fixup_thread_function_bookkeeping();

bool destroy = false;
{
MutexLock lock(GDScript::func_ptrs_to_update_thread_local->mutex);
DEV_ASSERT(GDScript::func_ptrs_to_update_thread_local->rc != 0);
GDScript::func_ptrs_to_update_thread_local->rc--;
if (GDScript::func_ptrs_to_update_thread_local->rc == 0) {
destroy = true;
}
}
if (destroy) {
memdelete(GDScript::func_ptrs_to_update_thread_local);
}
}

void GDScriptLanguage::init() {
Expand Down Expand Up @@ -2177,6 +2154,8 @@ void GDScriptLanguage::init() {
_add_global(E.name, E.ptr);
}

GDScript::func_ptrs_to_update_thread_local = &GDScript::func_ptrs_to_update_main_thread;

#ifdef TESTS_ENABLED
GDScriptTests::GDScriptTestRunner::handle_cmdline();
#endif
Expand Down Expand Up @@ -2226,6 +2205,8 @@ void GDScriptLanguage::finish() {
}
script_list.clear();
function_list.clear();

DEV_ASSERT(GDScript::func_ptrs_to_update_main_thread.rc == 1);
}

void GDScriptLanguage::profiling_start() {
Expand Down
19 changes: 11 additions & 8 deletions modules/gdscript/gdscript.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,21 +121,23 @@ class GDScript : public Script {
struct UpdatableFuncPtr {
List<GDScriptFunction **> ptrs;
Mutex mutex;
bool initialized = false;
bool initialized : 1;
bool transferred : 1;
uint32_t rc = 1;
UpdatableFuncPtr() :
initialized(false), transferred(false) {}
};
struct UpdatableFuncPtrElement {
List<GDScriptFunction **>::Element *element = nullptr;
Mutex *mutex = nullptr;
UpdatableFuncPtr *func_ptr = nullptr;
};
static thread_local UpdatableFuncPtr func_ptrs_to_update_thread_local;
static thread_local LocalVector<List<UpdatableFuncPtr *>::Element> func_ptrs_to_update_entries_thread_local;
static UpdatableFuncPtr *func_ptrs_to_update_main_thread;
static UpdatableFuncPtr func_ptrs_to_update_main_thread;
static thread_local UpdatableFuncPtr *func_ptrs_to_update_thread_local;
List<UpdatableFuncPtr *> func_ptrs_to_update;
List<UpdatableFuncPtrElement> func_ptrs_to_update_elems;
Mutex func_ptrs_to_update_mutex;

List<UpdatableFuncPtrElement>::Element *_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr);
static void _remove_func_ptr_to_update(List<UpdatableFuncPtrElement>::Element *p_func_ptr_element);
UpdatableFuncPtrElement _add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr);
static void _remove_func_ptr_to_update(const UpdatableFuncPtrElement &p_func_ptr_element);

static void _fixup_thread_function_bookkeeping();

Expand Down Expand Up @@ -561,6 +563,7 @@ class GDScriptLanguage : public ScriptLanguage {

/* MULTITHREAD FUNCTIONS */

virtual void thread_enter() override;
virtual void thread_exit() override;

/* DEBUGGER FUNCTIONS */
Expand Down
4 changes: 1 addition & 3 deletions modules/gdscript/gdscript_lambda_callable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,5 @@ GDScriptLambdaSelfCallable::GDScriptLambdaSelfCallable(Object *p_self, GDScriptF
}

GDScriptLambdaSelfCallable::~GDScriptLambdaSelfCallable() {
if (updatable_func_ptr_element) {
GDScript::_remove_func_ptr_to_update(updatable_func_ptr_element);
}
GDScript::_remove_func_ptr_to_update(updatable_func_ptr_element);
}
4 changes: 2 additions & 2 deletions modules/gdscript/gdscript_lambda_callable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class GDScriptLambdaCallable : public CallableCustom {
GDScriptFunction *function = nullptr;
Ref<GDScript> script;
uint32_t h;
List<GDScript::UpdatableFuncPtrElement>::Element *updatable_func_ptr_element = nullptr;
GDScript::UpdatableFuncPtrElement updatable_func_ptr_element;

Vector<Variant> captures;

Expand All @@ -72,7 +72,7 @@ class GDScriptLambdaSelfCallable : public CallableCustom {
Ref<RefCounted> reference; // For objects that are RefCounted, keep a reference.
Object *object = nullptr; // For non RefCounted objects, use a direct pointer.
uint32_t h;
List<GDScript::UpdatableFuncPtrElement>::Element *updatable_func_ptr_element = nullptr;
GDScript::UpdatableFuncPtrElement updatable_func_ptr_element;

Vector<Variant> captures;

Expand Down