diff --git a/CHANGELOG.md b/CHANGELOG.md index 8def114fe..f842f2021 100644 --- 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 index 2e2cab505..3b75cb82e 100644 --- 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 index ba246ab2e..1e7f15e06 100644 --- 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 index 682325f71..4bfffbc30 100644 --- a/src/sentry_value.c +++ b/src/sentry_value.c @@ -629,8 +629,22 @@ 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) { @@ -638,31 +652,24 @@ sentry__value_append_bounded(sentry_value_t value, sentry_value_t v, size_t max) } 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 index 437789940..9cfaddd86 100644 --- 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 index 8b03565c1..8e738bc1d 100644 --- 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 index 97f032fca..e81c34205 100644 --- 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)