adding release note, removing nframe, refactoring code and resolving unused checks

Author: alyshawang

ddtrace/profiling/collector/_memalloc.pyi

@@ -6,8 +6,8 @@ from .. import event
 FrameType = event.DDFrame
 StackType = event.StackTraceType
 
-# (stack, nframe, thread_id)
-TracebackType = typing.Tuple[StackType, int, int]
+# (stack, thread_id)
+TracebackType = typing.Tuple[StackType, int]
 
 def start(max_nframe: int, max_events: int, heap_sample_size: int) -> None: ...
 def stop() -> None: ...

ddtrace/profiling/collector/_memalloc_heap.c

@@ -80,7 +80,7 @@ typedef struct
         ptr_array_t frees;
     } freezer;
     /* List of freed samples that haven't been reported yet */
-    traceback_array_t allocation_list;
+    traceback_array_t unreported_samples;
 
     /* Debug guard to assert that GIL-protected critical sections are maintained
      * while accessing the profiler's state */
@@ -111,7 +111,7 @@ heap_tracker_init(heap_tracker_t* heap_tracker)
     heap_tracker->allocs_m = memalloc_heap_map_new();
     heap_tracker->freezer.allocs_m = memalloc_heap_map_new();
     ptr_array_init(&heap_tracker->freezer.frees);
-    traceback_array_init(&heap_tracker->allocation_list);
+    traceback_array_init(&heap_tracker->unreported_samples);
     heap_tracker->allocated_memory = 0;
     heap_tracker->frozen = false;
     heap_tracker->sample_size = 0;
@@ -125,7 +125,7 @@ heap_tracker_wipe(heap_tracker_t* heap_tracker)
     memalloc_heap_map_delete(heap_tracker->allocs_m);
     memalloc_heap_map_delete(heap_tracker->freezer.allocs_m);
     ptr_array_wipe(&heap_tracker->freezer.frees);
-    traceback_array_wipe(&heap_tracker->allocation_list);
+    traceback_array_wipe(&heap_tracker->unreported_samples);
 }
 
 static void
@@ -220,7 +220,7 @@ memalloc_heap_untrack_no_cpython(heap_tracker_t* heap_tracker, void* ptr)
         traceback_t* tb = memalloc_heap_map_remove(heap_tracker->allocs_m, ptr);
         if (tb && !tb->reported) {
             /* If the sample hasn't been reported yet, add it to the allocation list */
-            traceback_array_append(&heap_tracker->allocation_list, tb);
+            traceback_array_append(&heap_tracker->unreported_samples, tb);
             MEMALLOC_GIL_DEBUG_CHECK_RELEASE(&heap_tracker->gil_guard);
             return NULL;
         }
@@ -352,6 +352,36 @@ memalloc_heap_track(uint16_t max_nframe, void* ptr, size_t size, PyMemAllocatorD
     memalloc_yield_guard();
 }
 
+PyObject*
+memalloc_sample_to_tuple(traceback_t* tb, bool is_live)
+{
+    PyObject* tb_and_info = PyTuple_New(4);
+    if (tb_and_info == NULL) {
+        return NULL;
+    }
+
+    size_t in_use_size;
+    size_t alloc_size;
+
+    if (is_live) {
+        /* alloc_size tracks new allocations since the last heap snapshot. Once
+         * we report it (tb->reported == true), we set the value to 0 to avoid
+         * double-counting allocations across multiple snapshots. */
+        in_use_size = tb->size;
+        alloc_size = tb->reported ? 0 : tb->size;
+    } else {
+        in_use_size = 0;
+        alloc_size = tb->size;
+    }
+
+    PyTuple_SET_ITEM(tb_and_info, 0, traceback_to_tuple(tb));
+    PyTuple_SET_ITEM(tb_and_info, 1, PyLong_FromSize_t(in_use_size));
+    PyTuple_SET_ITEM(tb_and_info, 2, PyLong_FromSize_t(alloc_size));
+    PyTuple_SET_ITEM(tb_and_info, 3, PyLong_FromSize_t(tb->count));
+
+    return tb_and_info;
+}
+
 PyObject*
 memalloc_heap(void)
 {
@@ -364,7 +394,7 @@ memalloc_heap(void)
 
     /* Calculate total number of samples: live + freed */
     size_t live_count = memalloc_heap_map_size(global_heap_tracker.allocs_m);
-    size_t freed_count = global_heap_tracker.allocation_list.count;
+    size_t freed_count = global_heap_tracker.unreported_samples.count;
     size_t total_count = live_count + freed_count;
 
     PyObject* heap_list = PyList_New(total_count);
@@ -377,77 +407,43 @@ memalloc_heap(void)
 
     /* First, iterate over live samples using the new iterator API */
     memalloc_heap_map_iter_t* it = memalloc_heap_map_iter_new(global_heap_tracker.allocs_m);
-    if (it) {
-        void* key;
-        traceback_t* tb;
-
-        while (memalloc_heap_map_iter_next(it, &key, &tb)) {
-            if (list_index >= total_count) {
-                break;
-            }
-
-            PyObject* tb_and_info = PyTuple_New(4);
-            if (tb_and_info == NULL) {
-                continue;
-            }
-
-            size_t in_use_size = tb->size;
-            size_t alloc_size = tb->reported ? 0 : tb->size;
+    if (it == NULL) {
+        // TODO: return
+    }
 
-            PyTuple_SET_ITEM(tb_and_info, 0, traceback_to_tuple(tb));
-            PyTuple_SET_ITEM(tb_and_info, 1, PyLong_FromSize_t(in_use_size));
-            PyTuple_SET_ITEM(tb_and_info, 2, PyLong_FromSize_t(alloc_size));
-            PyTuple_SET_ITEM(tb_and_info, 3, PyLong_FromSize_t(tb->count));
+    void* key;
+    traceback_t* tb;
 
-            PyList_SET_ITEM(heap_list, list_index, tb_and_info);
-            list_index++;
+    while (memalloc_heap_map_iter_next(it, &key, &tb)) {
+        PyObject* tb_and_info = memalloc_sample_to_tuple(tb, true);
 
-            /* Mark as reported */
-            tb->reported = true;
-        }
+        PyList_SET_ITEM(heap_list, list_index, tb_and_info);
+        list_index++;
 
-        memalloc_heap_map_iter_delete(it);
+        /* Mark as reported */
+        tb->reported = true;
     }
 
-    /* Second, iterate over freed samples from allocation_list */
-    for (size_t i = 0; i < global_heap_tracker.allocation_list.count; i++) {
-        if (list_index >= total_count) {
-            break;
-        }
+    memalloc_heap_map_iter_delete(it);
 
-        traceback_t* tb = global_heap_tracker.allocation_list.tab[i];
+    /* Second, iterate over freed samples from unreported_samples */
+    for (size_t i = 0; i < global_heap_tracker.unreported_samples.count; i++) {
+        traceback_t* tb = global_heap_tracker.unreported_samples.tab[i];
 
-        PyObject* tb_and_info = PyTuple_New(4);
-        if (tb_and_info == NULL) {
-            continue;
-        }
-
-        size_t in_use_size = 0;
-        size_t alloc_size = tb->size;
-
-        PyTuple_SET_ITEM(tb_and_info, 0, traceback_to_tuple(tb));
-        PyTuple_SET_ITEM(tb_and_info, 1, PyLong_FromSize_t(in_use_size));
-        PyTuple_SET_ITEM(tb_and_info, 2, PyLong_FromSize_t(alloc_size));
-        PyTuple_SET_ITEM(tb_and_info, 3, PyLong_FromSize_t(tb->count));
+        PyObject* tb_and_info = memalloc_sample_to_tuple(tb, false);
 
         PyList_SET_ITEM(heap_list, list_index, tb_and_info);
         list_index++;
     }
 
-    if (list_index < total_count) {
-        if (PyList_SetSlice(heap_list, list_index, total_count, NULL) < 0) {
-            PyErr_Clear();
-        }
-    }
-
-    /* Free all tracebacks in allocation_list after reporting them */
-    for (size_t i = 0; i < global_heap_tracker.allocation_list.count; i++) {
-        if (global_heap_tracker.allocation_list.tab[i] != NULL) {
-            traceback_free(global_heap_tracker.allocation_list.tab[i]);
+    /* Free all tracebacks in unreported_samples after reporting them */
+    for (size_t i = 0; i < global_heap_tracker.unreported_samples.count; i++) {
+        if (global_heap_tracker.unreported_samples.tab[i] != NULL) {
+            traceback_free(global_heap_tracker.unreported_samples.tab[i]);
         }
     }
     /* Reset the count to 0 so we can reuse the memory */
-    global_heap_tracker.allocation_list.count = 0;
+    global_heap_tracker.unreported_samples.count = 0;
 
     heap_tracker_thaw(&global_heap_tracker);
 

ddtrace/profiling/collector/_memalloc_tb.c

@@ -253,7 +253,7 @@ memalloc_get_traceback(uint16_t max_nframe,
                        void* ptr,
                        size_t size,
                        PyMemAllocatorDomain domain,
-                       size_t allocated_memory)
+                       size_t weighted_size)
 {
     PyThreadState* tstate = PyThreadState_Get();
 
@@ -274,7 +274,7 @@ memalloc_get_traceback(uint16_t max_nframe,
     if (traceback == NULL)
         return NULL;
 
-    traceback->size = allocated_memory;
+    traceback->size = weighted_size;
     traceback->ptr = ptr;
 
     traceback->thread_id = PyThread_get_thread_ident();
@@ -283,15 +283,8 @@ memalloc_get_traceback(uint16_t max_nframe,
 
     traceback->reported = false;
 
-    if (size > 0) {
-        double scaled_count = ((double)allocated_memory) / ((double)size);
-        traceback->count = (size_t)scaled_count;
-        if (traceback->count == 0) {
-            traceback->count = 1;
-        }
-    } else {
-        traceback->count = 1;
-    }
+    double scaled_count = ((double)weighted_size) / ((double)size);
+    traceback->count = (size_t)scaled_count;
 
     return traceback;
 }
@@ -331,9 +324,8 @@ traceback_to_tuple(traceback_t* tb)
         PyTuple_SET_ITEM(stack, nframe, frame_tuple);
     }
 
-    PyObject* tuple = PyTuple_New(3);
+    PyObject* tuple = PyTuple_New(2);
     PyTuple_SET_ITEM(tuple, 0, stack);
-    PyTuple_SET_ITEM(tuple, 1, PyLong_FromUnsignedLong(tb->total_nframe));
-    PyTuple_SET_ITEM(tuple, 2, PyLong_FromUnsignedLong(tb->thread_id));
+    PyTuple_SET_ITEM(tuple, 1, PyLong_FromUnsignedLong(tb->thread_id));
     return tuple;
 }

ddtrace/profiling/collector/memalloc.py

@@ -23,7 +23,7 @@
 
 MemorySample = namedtuple(
     "MemorySample",
-    ("frames", "size", "count", "in_use_size", "alloc_size", "nframe", "thread_id"),
+    ("frames", "size", "count", "in_use_size", "alloc_size", "thread_id"),
 )
 
 
@@ -73,7 +73,7 @@ def on_shutdown():
             try:
                 _memalloc.stop()
             except RuntimeError:
-                LOG.debug("Failed to stop memalloc on shutdown", exc_info=True)
+                LOG.debug("Failed to stop memalloc profiling on shutdown", exc_info=True)
 
     def _get_thread_id_ignore_set(self):
         # type: () -> typing.Set[int]
@@ -96,7 +96,7 @@ def snapshot(self):
             return tuple()
 
         for event in events:
-            (frames, _, thread_id), in_use_size, alloc_size, count = event
+            (frames, thread_id), in_use_size, alloc_size, count = event
 
             if not self.ignore_profiler or thread_id not in thread_id_ignore_set:
                 handle = ddup.SampleHandle()
@@ -133,12 +133,12 @@ def test_snapshot(self):
 
         samples = []
         for event in events:
-            (frames, nframe, thread_id), in_use_size, alloc_size, count = event
+            (frames, thread_id), in_use_size, alloc_size, count = event
 
             if not self.ignore_profiler or thread_id not in thread_id_ignore_set:
                 size = in_use_size if in_use_size > 0 else alloc_size
 
-                samples.append(MemorySample(frames, size, count, in_use_size, alloc_size, nframe, thread_id))
+                samples.append(MemorySample(frames, size, count, in_use_size, alloc_size, thread_id))
 
         return tuple(samples)
 

releasenotes/notes/memory_profiler_unification-6478c2656a7adbfd.yaml

@@ -0,0 +1,7 @@
+---
+other:
+  - |
+    profiling: removed redundant sampling code from memory profile, improving overhead and accuracy.
+    Sizes and counts of objects allocated since the last profile are now reported more accurately.
+    ENV: DD_PROFILING_MAX_EVENTS is deprecated and does nothing. Use DD_PROFILING_HEAP_SAMPLE_SIZE
+    to control sampling frequency of the memory profiler.

tests/profiling/collector/test_memalloc.py

@@ -84,12 +84,10 @@ def test_iter_events():
         object_count = 0
         for sample in alloc_samples:
             stack = sample.frames
-            nframe = sample.nframe
             thread_id = sample.thread_id
             size = sample.alloc_size
 
             assert 0 < len(stack) <= max_nframe
-            assert nframe >= len(stack)
             assert size >= 1  # size depends on the object size
             last_call = stack[0]
             if last_call == DDFrame(
@@ -158,12 +156,10 @@ def test_iter_events_multi_thread():
         count_thread = 0
         for sample in alloc_samples:
             stack = sample.frames
-            nframe = sample.nframe
             thread_id = sample.thread_id
             size = sample.alloc_size
 
             assert 0 < len(stack) <= max_nframe
-            assert nframe >= len(stack)
             assert size >= 1  # size depends on the object size
             last_call = stack[0]
             if last_call == DDFrame(

tests/profiling_v2/collector/test_memalloc.py

@@ -197,7 +197,7 @@ def __init__(self, count, size):
 def get_heap_info(heap, funcs):
     got = {}
     for event in heap:
-        (frames, _, _), in_use_size, alloc_size, count = event
+        (frames, _), in_use_size, alloc_size, count = event
 
         in_use = in_use_size > 0
         size = in_use_size if in_use_size > 0 else alloc_size