Merge pull request #469 from DataDog/pawel/fix_memory_leak_in_stack_a…

…llocation Fix Memory leak when garbage collecting empty span stacks
DataDog · Jun 5, 2019 · 1476d15 · 1476d15
2 parents 37c0a21 + 143cdbf
commit 1476d15
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file - [read more
 
 ## [Unreleased]
 
+### Fixed
+- Memory leak when garbage collecting span stacks #469
+
 ## [0.27.0]
 
 ### Added

diff --git a/src/ext/coms.c b/src/ext/coms.c
@@ -55,7 +55,7 @@ static uint32_t store_data(group_id_t group_id, const char *src, size_t size) {
     return 0;
 }
 
-ddtrace_coms_stack_t *new_stack() {
+static inline ddtrace_coms_stack_t *new_stack() {
     ddtrace_coms_stack_t *stack = calloc(1, sizeof(ddtrace_coms_stack_t));
     stack->size = DD_TRACE_COMS_STACK_SIZE;
     stack->data = calloc(1, stack->size);
@@ -68,7 +68,7 @@ void ddtrace_coms_free_stack(ddtrace_coms_stack_t *stack) {
     free(stack);
 }
 
-void recycle_stack(ddtrace_coms_stack_t *stack) {
+static inline void recycle_stack(ddtrace_coms_stack_t *stack) {
     char *data = stack->data;
     size_t size = stack->size;
 
@@ -79,14 +79,14 @@ void recycle_stack(ddtrace_coms_stack_t *stack) {
     stack->size = size;
 }
 
-void gc_stacks() {
+static inline void gc_stacks() {
     for (int i = 0; i < DD_TRACE_COMS_STACKS_BACKLOG_SIZE; i++) {
         ddtrace_coms_stack_t *stack = ddtrace_coms_global_state.stacks[i];
 
         if (stack) {
-            if (ddtrace_coms_is_stack_unused(stack) && atomic_load(&stack->bytes_written) == 0) {
+            if (ddtrace_coms_is_stack_free(stack)) {
                 ddtrace_coms_global_state.stacks[i] = NULL;
-                free(stack);
+                ddtrace_coms_free_stack(stack);
             } else {
                 stack->gc_cycles_count++;
             }
@@ -107,26 +107,30 @@ BOOL_T ddtrace_coms_initialize() {
 }
 
 BOOL_T ddtrace_coms_rotate_stack() {
-    ddtrace_coms_stack_t *stack = NULL;
+    ddtrace_coms_stack_t *next_stack = NULL;
     ddtrace_coms_stack_t *current_stack = atomic_load(&ddtrace_coms_global_state.current_stack);
+    if (current_stack && ddtrace_coms_is_stack_free(current_stack)) {
+        return 0;  // stack is empty and unusued - no need to swap it out
+    }
 
-    for (int i = 0; i < DD_TRACE_COMS_STACKS_BACKLOG_SIZE; i++) {
-        ddtrace_coms_stack_t *stack_tmp = ddtrace_coms_global_state.stacks[i];
-        if (stack_tmp) {
-            if (atomic_load(&stack_tmp->refcount) == 0 && atomic_load(&stack_tmp->bytes_written) == 0) {
-                stack = stack_tmp;
+    if (current_stack) {
+        // try to swap out current stack for an empty stack
+        for (int i = 0; i < DD_TRACE_COMS_STACKS_BACKLOG_SIZE; i++) {
+            ddtrace_coms_stack_t *stack_tmp = ddtrace_coms_global_state.stacks[i];
+            if (stack_tmp && ddtrace_coms_is_stack_free(stack_tmp)) {
+                next_stack = stack_tmp;
                 recycle_stack(stack_tmp);
                 ddtrace_coms_global_state.stacks[i] = current_stack;
                 current_stack = NULL;
                 break;
             }
         }
     }
-
     // attempt to freeup stack storage
     gc_stacks();
 
     if (current_stack != NULL) {
+        // try to store current stack in an empty slot
         for (int i = 0; i < DD_TRACE_COMS_STACKS_BACKLOG_SIZE; i++) {
             if (!ddtrace_coms_global_state.stacks[i]) {
                 ddtrace_coms_global_state.stacks[i] = current_stack;
@@ -137,14 +141,15 @@ BOOL_T ddtrace_coms_rotate_stack() {
 
     // old current stack was stored so set a new stack
     if (current_stack == NULL) {
-        if (!stack) {
-            stack = new_stack();
+        if (!next_stack) {
+            next_stack = new_stack();
         }
 
-        atomic_store(&ddtrace_coms_global_state.current_stack, stack);
+        atomic_store(&ddtrace_coms_global_state.current_stack, next_stack);
         return 0;
     }
-    // if we couldn't store new stack i tem
+
+    // we couldn't store old stack so we cannot provide new empty stack
     return ENOMEM;
 }
 

diff --git a/src/ext/coms.h b/src/ext/coms.h
@@ -5,7 +5,7 @@
 #include "env_config.h"
 #include "vendor_stdatomic.h"
 
-#define DD_TRACE_COMS_STACK_SIZE (1024 * 1024 * 10)  // 5 MB
+#define DD_TRACE_COMS_STACK_SIZE (1024 * 1024 * 10)  // 10 MB
 #define DD_TRACE_COMS_STACKS_BACKLOG_SIZE 10
 
 typedef struct ddtrace_coms_stack_t {
@@ -23,10 +23,14 @@ typedef struct ddtrace_coms_state_t {
     _Atomic(uint32_t) next_group_id;
 } ddtrace_coms_state_t;
 
-inline static uint32_t ddtrace_coms_is_stack_unused(ddtrace_coms_stack_t *stack) {
+inline static BOOL_T ddtrace_coms_is_stack_unused(ddtrace_coms_stack_t *stack) {
     return atomic_load(&stack->refcount) == 0;
 }
 
+inline static BOOL_T ddtrace_coms_is_stack_free(ddtrace_coms_stack_t *stack) {
+    return ddtrace_coms_is_stack_unused(stack) && atomic_load(&stack->bytes_written) == 0;
+}
+
 BOOL_T ddtrace_coms_rotate_stack();
 ddtrace_coms_stack_t *ddtrace_coms_attempt_acquire_stack();
 

diff --git a/tests/ext/dd_trace_coms_test_parallel_writer_consistency.phpt b/tests/ext/dd_trace_coms_test_parallel_writer_consistency.phpt
@@ -46,7 +46,6 @@ bar
 bytes_written 4400030
 a
 bytes_written 13
-bytes_written 0
 foo
 bar
 bytes_written 4400030