From be13df2abbb323791c4d04c75a98c5df7a92c9ab Mon Sep 17 00:00:00 2001
From: Justin Ethier <justin.ethier@gmail.com>
Date: Mon, 21 Dec 2015 22:57:36 -0500
Subject: [PATCH] Perform deferred free of thread data

Mark a mutator's thread as old when a thread is terminated, and free it at a later time. This is done to prevent race conditions with the collector thread, which could be in the middle of working with a thread's data.
---
 gc.c      | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 runtime.c |  7 ++-----
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/gc.c b/gc.c
index 34d27d03..42ff0c0a 100644
--- a/gc.c
+++ b/gc.c
@@ -42,7 +42,7 @@ static int cached_heap_free_size = 0;
 static int cached_heap_total_size = 0;
 
 // Data for each individual mutator thread
-ck_array_t Cyc_mutators;
+ck_array_t Cyc_mutators, old_mutators;
 static pthread_mutex_t mutators_lock;
 
 static void my_free(void *p, size_t m, bool d)
@@ -78,6 +78,11 @@ void gc_initialize()
     exit(1);
   }
 
+  if (ck_array_init(&old_mutators, CK_ARRAY_MODE_SPMC, &my_allocator, 10) == 0){
+    fprintf(stderr, "Unable to initialize mutator array\n");
+    exit(1);
+  }
+
   // Initialize collector's mark stack
   mark_stack_len = 128;
   mark_stack = vpbuffer_realloc(mark_stack, &(mark_stack_len));
@@ -105,6 +110,10 @@ void gc_add_mutator(gc_thread_data *thd)
   pthread_mutex_unlock(&mutators_lock);
 }
 
+// Remove selected mutator from the mutator list.
+// This is done for terminated threads. Note data is queued to be
+// freed, to prevent accidentally freeing it while the collector
+// thread is potentially accessing it.
 void gc_remove_mutator(gc_thread_data *thd)
 {
   pthread_mutex_lock(&mutators_lock);
@@ -113,6 +122,36 @@ void gc_remove_mutator(gc_thread_data *thd)
     exit(1);
   }
   ck_array_commit(&Cyc_mutators);
+  // Place on list of old mutators to cleanup
+  if (ck_array_put_unique(&old_mutators, (void *)thd) < 0) {
+    fprintf(stderr, "Unable to add thread data to GC list, existing\n");
+    exit(1);
+  }
+  ck_array_commit(&old_mutators);
+  pthread_mutex_unlock(&mutators_lock);
+}
+
+void gc_free_old_thread_data()
+{
+  ck_array_iterator_t iterator;
+  gc_thread_data *m;
+  int freed = 0;
+
+  pthread_mutex_lock(&mutators_lock);
+  CK_ARRAY_FOREACH(&old_mutators, &iterator, &m){
+printf("JAE DEBUG - freeing old thread data...");
+    gc_thread_data_free(m);
+    if (!ck_array_remove(&old_mutators, (void *)m)) {
+      fprintf(stderr, "Error removing old mutator data\n");
+      exit(1);
+    }
+    freed = 1;
+printf(" done\n");
+  }
+  if (freed) {
+    ck_array_commit(&old_mutators);
+printf("commited old mutator data deletions\n");
+  }
   pthread_mutex_unlock(&mutators_lock);
 }
 
@@ -1195,6 +1234,11 @@ fprintf(stderr, "DEBUG - after wait_handshake async\n");
     total_size, total_free, 
     freed, max_freed, time(NULL) - sweep_start);
 #endif
+#if GC_DEBUG_TRACE
+  fprintf(stderr, "cleaning up any old thread data\n");
+#endif
+  gc_free_old_thread_data();
+  // Idle the GC thread
   ATOMIC_SET_IF_EQ(&gc_stage, STAGE_SWEEPING, STAGE_RESTING);
 }
 
diff --git a/runtime.c b/runtime.c
index 5bfcb6f6..82db23c9 100644
--- a/runtime.c
+++ b/runtime.c
@@ -2811,13 +2811,10 @@ void Cyc_exit_thread(gc_thread_data *thd)
   // referenced? might want to do one more minor GC to clear the stack before
   // terminating the thread
 
-printf("DEBUG - exiting thread\n");
-// TODO: race condition, cannot free thread data if the collector is using it
+//printf("DEBUG - exiting thread\n");
+  // Remove thread from the list of mutators, and mark its data to be freed
   gc_remove_mutator(thd);
   ATOMIC_SET_IF_EQ(&(thd->thread_state), CYC_THREAD_STATE_RUNNABLE, CYC_THREAD_STATE_TERMINATED);
-// TODO: could maintain a dedicated list of old thread data to clean up...
-//  collector could do it at a time that is safe
-//  gc_thread_data_free(thd);
   pthread_exit(NULL); // For now, just a proof of concept
 }