feat: add breadcrumb ringbuffer to avoid O(n) memmove (#1060)

* feat: initial ringbuffer implementation * chore: cleanup code * chore: added todo * removed unnecessary buffer end value * changed buffer start_idx storage to [0] * fixed issues in new storing method * updated test to new ringbuffer append logic * refactor: renamed to sentry__value_append_ringbuffer * test: removed old bounded append test * chore: update CHANGELOG.md * chore: update CHANGELOG.md * chore: linting * increase refcount of ringbuffer-to-list items * apply suggestion from code review * added ringbuffer test * fixed ringbuffer to list conversion * direct access to ringbuffer items * updated test with proper refcount check * removed unnecessary decref from test * added decref of temporary ringbuffer list * removed double cloning * changing types from int32_t to size_t * moved declaration from public to private header * added decref * type conversion * applied suggestions from code review
getsentry · Nov 4, 2024 · ffdf6ed · ffdf6ed
1 parent 341a64f
commit ffdf6ed
Show file tree

Hide file tree

Showing 7 changed files with 130 additions and 88 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,12 @@
 # Changelog
 
+## Unreleased
+
+
+**Fixes**:
+
+- Add breadcrumb ringbuffer to avoid O(n) memmove on adding more than max breadcrumbs ([#1060](https://github.com/getsentry/sentry-native/pull/1060))
+
 ## 0.7.11
 
 **Fixes**:

diff --git a/src/sentry_core.c b/src/sentry_core.c
@@ -652,7 +652,7 @@ sentry_add_breadcrumb(sentry_value_t breadcrumb)
     // the `no_flush` will avoid triggering *both* scope-change and
     // breadcrumb-add events.
     SENTRY_WITH_SCOPE_MUT_NO_FLUSH (scope) {
-        sentry__value_append_bounded(
+        sentry__value_append_ringbuffer(
             scope->breadcrumbs, breadcrumb, max_breadcrumbs);
     }
 }

diff --git a/src/sentry_scope.c b/src/sentry_scope.c
@@ -337,7 +337,10 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope,
     sentry_value_decref(contexts);
 
     if (mode & SENTRY_SCOPE_BREADCRUMBS) {
-        PLACE_CLONED_VALUE("breadcrumbs", scope->breadcrumbs);
+        sentry_value_t l
+            = sentry__value_ring_buffer_to_list(scope->breadcrumbs);
+        PLACE_VALUE("breadcrumbs", l);
+        sentry_value_decref(l);
     }
 
     if (mode & SENTRY_SCOPE_MODULES) {

diff --git a/src/sentry_value.c b/src/sentry_value.c
@@ -629,40 +629,47 @@ sentry__value_clone(sentry_value_t value)
     }
 }
 
+/**
+ * This appends `v` to the List `value`.
+ * To make this work properly as a ring buffer, the value list needs to have
+ * the ring buffer start index as the first element
+ * (e.g, 1 until max is exceeded, then it will update for each added item)
+ *
+ * It will remove the oldest value in the list, in case the total number of
+ * items would exceed `max`.
+ *
+ * The list is of size `max + 1` to store the start index.
+ *
+ * Returns 0 on success.
+ */
 int
-sentry__value_append_bounded(sentry_value_t value, sentry_value_t v, size_t max)
+sentry__value_append_ringbuffer(
+    sentry_value_t value, sentry_value_t v, size_t max)
 {
     thing_t *thing = value_as_unfrozen_thing(value);
     if (!thing || thing_get_type(thing) != THING_TYPE_LIST) {
         goto fail;
     }
 
     list_t *l = thing->payload._ptr;
-
-    if (l->len < max) {
+    if (l->len == 0) {
+        sentry_value_append(value, sentry_value_new_int32(1));
+    }
+    if (l->len < max + 1) {
         return sentry_value_append(value, v);
     }
+    if (l->len > max + 1) {
+        SENTRY_WARNF("Cannot reduce Ringbuffer list size from %d to %d.",
+            l->len - 1, max);
+        goto fail;
+    }
+    const int32_t start_idx = sentry_value_as_int32(l->items[0]);
 
-    // len: 120
-    // max: 100
-    // move to 0
-    //   move 99 items (len - 1)
-    //   from 20
+    sentry_value_decref(l->items[start_idx]);
+    l->items[start_idx] = v;
+    l->items[0] = sentry_value_new_int32((start_idx % (int32_t)max) + 1);
 
-    size_t to_move = max >= 1 ? max - 1 : 0;
-    size_t to_shift = l->len - to_move;
-    for (size_t i = 0; i < to_shift; i++) {
-        sentry_value_decref(l->items[i]);
-    }
-    if (to_move) {
-        memmove(l->items, l->items + to_shift, to_move * sizeof(l->items[0]));
-    }
-    if (max >= 1) {
-        l->items[max - 1] = v;
-    } else {
-        sentry_value_decref(v);
-    }
-    l->len = max;
+    l->len = max + 1;
     return 0;
 
 fail:
@@ -840,6 +847,28 @@ sentry_value_as_string(sentry_value_t value)
     }
 }
 
+sentry_value_t
+sentry__value_ring_buffer_to_list(const sentry_value_t rb)
+{
+    const thing_t *thing = value_as_thing(rb);
+    if (!thing || thing_get_type(thing) != THING_TYPE_LIST) {
+        return sentry_value_new_null();
+    }
+    const list_t *rb_list = thing->payload._ptr;
+    if (rb_list->len == 0) {
+        return sentry_value_new_list();
+    }
+    const size_t start_idx = sentry_value_as_int32(rb_list->items[0]);
+
+    sentry_value_t rv = sentry_value_new_list();
+    for (size_t i = 0; i < rb_list->len - 1; i++) {
+        const size_t idx = (start_idx - 1 + i) % (rb_list->len - 1) + 1;
+        sentry_value_incref(rb_list->items[idx]);
+        sentry_value_append(rv, rb_list->items[idx]);
+    }
+    return rv;
+}
+
 int
 sentry_value_is_true(sentry_value_t value)
 {

diff --git a/src/sentry_value.h b/src/sentry_value.h
@@ -81,14 +81,23 @@ sentry_value_t sentry__value_clone(sentry_value_t value);
 
 /**
  * This appends `v` to the List `value`.
- * It will remove the first value of the list, is case the total number if items
- * would exceed `max`.
+ *
+ * On non-full lists, exponentially reallocate space to accommodate new values
+ * (until we reach `max`). After reaching max, the oldest value is removed to
+ * make space for the new one.
+ *
+ * `max` should stay fixed over multiple invocations.
  *
  * Returns 0 on success.
  */
-int sentry__value_append_bounded(
+int sentry__value_append_ringbuffer(
     sentry_value_t value, sentry_value_t v, size_t max);
 
+/**
+ * Converts ring buffer to linear list
+ */
+sentry_value_t sentry__value_ring_buffer_to_list(sentry_value_t rb);
+
 /**
  * Deep-merges object src into dst.
  *

diff --git a/tests/unit/test_value.c b/tests/unit/test_value.c
@@ -176,21 +176,68 @@ SENTRY_TEST(value_list)
     sentry_value_decref(val);
 
     val = sentry_value_new_list();
+    sentry_value_append(val, sentry_value_new_int32(1));
     for (int32_t i = 1; i <= 10; i++) {
-        sentry_value_append(val, sentry_value_new_int32(i));
+        sentry__value_append_ringbuffer(val, sentry_value_new_int32(i), 5);
     }
-    sentry__value_append_bounded(val, sentry_value_new_int32(1010), 5);
+    sentry__value_append_ringbuffer(val, sentry_value_new_int32(1010), 5);
 #define CHECK_IDX(Idx, Val)                                                    \
     TEST_CHECK_INT_EQUAL(                                                      \
         sentry_value_as_int32(sentry_value_get_by_index(val, Idx)), Val)
-    CHECK_IDX(0, 7);
-    CHECK_IDX(1, 8);
-    CHECK_IDX(2, 9);
-    CHECK_IDX(3, 10);
-    CHECK_IDX(4, 1010);
+    CHECK_IDX(1, 1010);
+    CHECK_IDX(2, 7);
+    CHECK_IDX(3, 8);
+    CHECK_IDX(4, 9);
+    CHECK_IDX(5, 10);
     sentry_value_decref(val);
 }
 
+SENTRY_TEST(value_ringbuffer)
+{
+    sentry_value_t val = sentry_value_new_list();
+    sentry_value_append(val, sentry_value_new_int32(1)); // start index
+
+    const sentry_value_t v0 = sentry_value_new_object();
+    sentry_value_set_by_key(v0, "key", sentry_value_new_int32((int32_t)0));
+    const sentry_value_t v1 = sentry_value_new_object();
+    sentry_value_set_by_key(v1, "key", sentry_value_new_int32((int32_t)1));
+    const sentry_value_t v2 = sentry_value_new_object();
+    sentry_value_set_by_key(v2, "key", sentry_value_new_int32((int32_t)2));
+    const sentry_value_t v3 = sentry_value_new_object();
+    sentry_value_set_by_key(v3, "key", sentry_value_new_int32((int32_t)3));
+
+    sentry__value_append_ringbuffer(val, v0, 3);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v0), 1);
+    sentry_value_incref(v0);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v0), 2);
+
+    sentry__value_append_ringbuffer(val, v1, 3);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v1), 1);
+    sentry__value_append_ringbuffer(val, v2, 3);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v2), 1);
+    sentry__value_append_ringbuffer(val, v3, 3);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v3), 1);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v0), 1);
+
+    const sentry_value_t l = sentry__value_ring_buffer_to_list(val);
+    TEST_CHECK_INT_EQUAL(sentry_value_get_length(l), 3);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v3), 2);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v2), 2);
+    TEST_CHECK_INT_EQUAL(sentry_value_refcount(v1), 2);
+#define CHECK_KEY_IDX(List, Idx, Val)                                          \
+    TEST_CHECK_INT_EQUAL(sentry_value_as_int32(sentry_value_get_by_key(        \
+                             sentry_value_get_by_index(List, Idx), "key")),    \
+        Val)
+
+    CHECK_KEY_IDX(l, 0, 1);
+    CHECK_KEY_IDX(l, 1, 2);
+    CHECK_KEY_IDX(l, 2, 3);
+
+    sentry_value_decref(l);
+    sentry_value_decref(val);
+    sentry_value_decref(v0); // one manual incref
+}
+
 SENTRY_TEST(value_object)
 {
     sentry_value_t val = sentry_value_new_object();
@@ -509,59 +556,6 @@ SENTRY_TEST(value_wrong_type)
     TEST_CHECK(sentry_value_get_length(val) == 0);
 }
 
-SENTRY_TEST(value_collections_leak)
-{
-    // decref the value correctly on error
-    sentry_value_t obj = sentry_value_new_object();
-    sentry_value_t null_v = sentry_value_new_null();
-
-    sentry_value_incref(obj);
-    sentry_value_set_by_key(null_v, "foo", obj);
-
-    sentry_value_incref(obj);
-    sentry_value_set_by_index(null_v, 123, obj);
-
-    sentry_value_incref(obj);
-    sentry_value_append(null_v, obj);
-
-    TEST_CHECK_INT_EQUAL(sentry_value_refcount(obj), 1);
-
-    sentry_value_t list = sentry_value_new_list();
-
-    sentry_value_incref(obj);
-    sentry_value_append(list, obj);
-    sentry_value_incref(obj);
-    sentry_value_append(list, obj);
-    sentry_value_incref(obj);
-    sentry_value_append(list, obj);
-    sentry_value_incref(obj);
-    sentry_value_append(list, obj);
-    sentry_value_incref(obj);
-    sentry_value_append(list, obj);
-
-    // decref the existing values correctly on bounded append
-    sentry_value_incref(obj);
-    sentry__value_append_bounded(list, obj, 2);
-    sentry_value_incref(obj);
-    sentry__value_append_bounded(list, obj, 2);
-
-    TEST_CHECK_INT_EQUAL(sentry_value_refcount(obj), 3);
-
-    sentry_value_incref(obj);
-    sentry__value_append_bounded(list, obj, 1);
-    TEST_CHECK_INT_EQUAL(sentry_value_refcount(obj), 2);
-
-    sentry_value_incref(obj);
-    sentry__value_append_bounded(list, obj, 0);
-    TEST_CHECK_INT_EQUAL(sentry_value_refcount(obj), 1);
-    TEST_CHECK_INT_EQUAL(sentry_value_get_length(list), 0);
-
-    sentry_value_decref(list);
-
-    TEST_CHECK_INT_EQUAL(sentry_value_refcount(obj), 1);
-    sentry_value_decref(obj);
-}
-
 SENTRY_TEST(value_set_by_null_key)
 {
     sentry_value_t value = sentry_value_new_object();

diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc
@@ -115,7 +115,6 @@ XX(user_feedback_with_null_args)
 XX(uuid_api)
 XX(uuid_v4)
 XX(value_bool)
-XX(value_collections_leak)
 XX(value_double)
 XX(value_freezing)
 XX(value_get_by_null_key)
@@ -132,6 +131,7 @@ XX(value_object)
 XX(value_object_merge)
 XX(value_object_merge_nested)
 XX(value_remove_by_null_key)
+XX(value_ringbuffer)
 XX(value_set_by_null_key)
 XX(value_set_stacktrace)
 XX(value_string)