From 4fa2630a5bf45b6164aa0989ae5f835cf6038b82 Mon Sep 17 00:00:00 2001 From: Calvin Ellison <32909984+celliso1@users.noreply.github.com> Date: Fri, 4 Sep 2026 09:43:23 -0700 Subject: [PATCH] [Core] Avoid O(n^2) header dedup when cloning EF_UNIQ_HEADERS events (#3061) * events: avoid O(n^2) header dedup when cloning EF_UNIQ_HEADERS events switch_event_dup() copies a source event header by header via switch_event_add_header_string(). When the source carries EF_UNIQ_HEADERS (SWITCH_EVENT_CHANNEL_DATA / REQUEST_PARAMS / MESSAGE -- e.g. a channel's variable list), the destination inherits the flag, so switch_event_base_add_header() runs a full switch_event_del_header() linear scan of the partially-built list on every add to enforce uniqueness. Cloning an N-header event is therefore O(n^2). That scan is redundant while cloning: the source already enforced uniqueness on insert, so copying it cannot introduce duplicates regardless of the per-add scan. Clear EF_UNIQ_HEADERS on the destination for the duration of the copy loop, then restore it. This is a hot path. switch_channel_execute_on() -- invoked on ring, answer, media, transfer, park, record and playback -- calls switch_core_get_variables() and switch_channel_get_variables(), each of which dup()s an EF_UNIQ_HEADERS event. On a production voicemail server carrying ~191 channel variables per call, switch_event_del_header_val() accounted for 47-69% of all FreeSWITCH CPU time in on-box perf profiles. Microbenchmark (tests/unit/switch_event.c, switch_event_dup of an N-header CHANNEL_DATA event, 4000 iterations, time per dup): N before after 50 4.80 us 3.19 us 100 11.34 us 5.88 us 191 28.29 us 10.99 us (2.6x) 400 107.05 us 23.27 us (4.6x) "before" scales ~O(n^2); "after" is linear. Behaviour is unchanged: the clone has identical headers, values and order, keeps EF_UNIQ_HEADERS, and still enforces uniqueness on subsequent adds (covered by the test). Signed-off-by: Calvin Ellison * tests: cover switch_event_dup uniqueness and add a dup benchmark Adds a dup_uniq_bench case to the switch_event unit test. For an EF_UNIQ_HEADERS (CHANNEL_DATA) source event it verifies the clone preserves header count, values and the EF_UNIQ_HEADERS flag, and that re-setting an existing key does not produce a duplicate in the clone. It also prints a switch_event_dup timing sweep across header counts, used to validate the O(n^2) -> O(n) change in switch_event_dup(). Signed-off-by: Calvin Ellison * tests: add dup_faithful_copy regression for switch_event_dup Covers the EF_UNIQ_HEADERS edge case for the O(n^2)->O(n) dup change: a well-formed EF_UNIQ source stays unique through dup, and a malformed source already holding duplicate names (only possible if headers were added before the flag was set) is copied faithfully rather than silently collapsed to the last value. Signed-off-by: Calvin Ellison * tests: gate dup_uniq_bench behind #ifdef BENCHMARK The dup_uniq_bench timing sweep is a benchmark rather than a pass/fail correctness test. Guard the whole test with #ifdef BENCHMARK (matching the existing benchmark in this file) so it stays out of normal test runs; dup_faithful_copy continues to cover correctness of the switch_event_dup change. Signed-off-by: Calvin Ellison * switch_event: clarify EF_UNIQ_HEADERS dup comment, brace one-line test loops Address review feedback on the switch_event_dup change: - src/switch_event.c: the fast-path comment claimed todup's headers are "already unique" unconditionally. That only holds when EF_UNIQ_HEADERS is set (names are deduped on insert); reword to scope the claim to that case and to the verbatim-copy intent. - tests/unit/switch_event.c: expand one-line loops to braced form per the SignalWire coding guidelines. Signed-off-by: Calvin Ellison --------- Signed-off-by: Calvin Ellison Co-authored-by: Calvin Ellison --- src/switch_event.c | 18 +++++ tests/unit/switch_event.c | 154 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+) diff --git a/src/switch_event.c b/src/switch_event.c index 9d8907d09a..701bda2325 100644 --- a/src/switch_event.c +++ b/src/switch_event.c @@ -1335,6 +1335,7 @@ SWITCH_DECLARE(void) switch_event_merge(switch_event_t *event, switch_event_t *t SWITCH_DECLARE(switch_status_t) switch_event_dup(switch_event_t **event, switch_event_t *todup) { switch_event_header_t *hp; + int restore_uniq_headers = 0; if (switch_event_create_subclass(event, SWITCH_EVENT_CLONE, todup->subclass_name) != SWITCH_STATUS_SUCCESS) { return SWITCH_STATUS_GENERR; @@ -1344,6 +1345,19 @@ SWITCH_DECLARE(switch_status_t) switch_event_dup(switch_event_t **event, switch_ (*event)->event_user_data = todup->event_user_data; (*event)->bind_user_data = todup->bind_user_data; (*event)->flags = todup->flags; + + /* The destination inherited todup's flags above. When EF_UNIQ_HEADERS is set, + * switch_event_base_add_header() rescans the partial header list on every add + * to keep names unique, making this copy loop O(n^2). A clone should reproduce + * todup's list verbatim rather than run a fresh uniqueness pass, and a + * well-formed EF_UNIQ_HEADERS source is already unique (its names were deduped + * on insert), so that scan only adds cost here. Suppress it for the copy, then + * restore the flag so later adds enforce uniqueness again. */ + if (switch_test_flag((*event), EF_UNIQ_HEADERS)) { + switch_clear_flag((*event), EF_UNIQ_HEADERS); + restore_uniq_headers = 1; + } + for (hp = todup->headers; hp; hp = hp->next) { if (todup->subclass_name && !strcmp(hp->name, "Event-Subclass")) { continue; @@ -1359,6 +1373,10 @@ SWITCH_DECLARE(switch_status_t) switch_event_dup(switch_event_t **event, switch_ } } + if (restore_uniq_headers) { + switch_set_flag((*event), EF_UNIQ_HEADERS); + } + if (todup->body) { (*event)->body = DUP(todup->body); } diff --git a/tests/unit/switch_event.c b/tests/unit/switch_event.c index 4ee17d5eb8..0268d5762b 100644 --- a/tests/unit/switch_event.c +++ b/tests/unit/switch_event.c @@ -161,6 +161,160 @@ FST_TEST_BEGIN(expand_headers_offset) } FST_TEST_END() +#ifdef BENCHMARK +FST_TEST_BEGIN(dup_uniq_bench) +{ + switch_event_t *src = NULL, *dup = NULL; + switch_status_t status; + const int loops = 4000; + const int Ns[4] = {50, 100, 191, 400}; + int i, j, count, k, S, Nv; + char name[32], val[64]; + switch_event_header_t *hp; + switch_time_t start_ts, end_ts; + double per_us; + + /* A channel's variables are a SWITCH_EVENT_CHANNEL_DATA event => EF_UNIQ_HEADERS. + * switch_event_create() also adds the standard event headers, so the source + * count is (191 added + standard); assert relative to the actual source count. */ + status = switch_event_create(&src, SWITCH_EVENT_CHANNEL_DATA); + fst_xcheck(status == SWITCH_STATUS_SUCCESS, "create CHANNEL_DATA event"); + fst_xcheck(switch_test_flag(src, EF_UNIQ_HEADERS) != 0, "CHANNEL_DATA event is EF_UNIQ_HEADERS"); + + for (i = 0; i < 191; i++) { + switch_snprintf(name, sizeof(name), "var_%d", i); + switch_snprintf(val, sizeof(val), "value_%d_some_padding_payload", i); + switch_event_add_header_string(src, SWITCH_STACK_BOTTOM, name, val); + } + S = 0; + for (hp = src->headers; hp; hp = hp->next) { + S++; + } + fst_xcheck(S >= 191, "source has at least the 191 added headers"); + + /* correctness: dup preserves header count, the uniq flag, and values */ + status = switch_event_dup(&dup, src); + fst_xcheck(status == SWITCH_STATUS_SUCCESS, "dup ok"); + count = 0; + for (hp = dup->headers; hp; hp = hp->next) { + count++; + } + fst_xcheck(count == S, "dup header count equals source"); + fst_xcheck(switch_test_flag(dup, EF_UNIQ_HEADERS) != 0, "dup keeps EF_UNIQ_HEADERS"); + fst_check_string_equals(switch_event_get_header(dup, "var_0"), "value_0_some_padding_payload"); + fst_check_string_equals(switch_event_get_header(dup, "var_190"), "value_190_some_padding_payload"); + switch_event_destroy(&dup); + + /* correctness: uniqueness preserved (re-setting a key must not duplicate it in the dup) */ + switch_event_add_header_string(src, SWITCH_STACK_BOTTOM, "var_0", "REPLACED"); + status = switch_event_dup(&dup, src); + fst_xcheck(status == SWITCH_STATUS_SUCCESS, "dup ok (2)"); + k = 0; + for (hp = dup->headers; hp; hp = hp->next) { + if (!strcmp(hp->name, "var_0")) { + k++; + } + } + fst_xcheck(k == 1, "var_0 present exactly once after re-set + dup"); + fst_check_string_equals(switch_event_get_header(dup, "var_0"), "REPLACED"); + switch_event_destroy(&dup); + switch_event_destroy(&src); + + /* timing sweep: dup cost vs header count (baseline ~O(n^2), patched ~O(n)) */ + for (j = 0; j < 4; j++) { + Nv = Ns[j]; + switch_event_create(&src, SWITCH_EVENT_CHANNEL_DATA); + for (i = 0; i < Nv; i++) { + switch_snprintf(name, sizeof(name), "var_%d", i); + switch_snprintf(val, sizeof(val), "value_%d_some_padding_payload", i); + switch_event_add_header_string(src, SWITCH_STACK_BOTTOM, name, val); + } + start_ts = switch_time_now(); + for (i = 0; i < loops; i++) { + switch_event_dup(&dup, src); + switch_event_destroy(&dup); + } + end_ts = switch_time_now(); + per_us = (double)(end_ts - start_ts) / (double)loops; + printf("DUP_BENCH N=%d loops=%d total=%" SWITCH_UINT64_T_FMT "us per_dup=%.3f us\n", + Nv, loops, (uint64_t)(end_ts - start_ts), per_us); + switch_event_destroy(&src); + } +} +FST_TEST_END() +#endif /* BENCHMARK */ + +FST_TEST_BEGIN(dup_faithful_copy) +{ + /* Regression for the switch_event_dup O(n^2)->O(n) change (suppressing the + * per-add EF_UNIQ_HEADERS dedup scan while cloning). dup must reproduce the + * source faithfully: + * (1) a well-formed EF_UNIQ source stays unique through dup (the real case); + * (2) a malformed source that already holds duplicate names (only possible + * if headers were added before EF_UNIQ_HEADERS was set) is copied + * faithfully -- dup PRESERVES the duplicates rather than silently + * collapsing to the last value. A dup-bearing EF_UNIQ event is already + * a bug upstream of here; a copy must not silently drop data. */ + switch_event_t *src = NULL, *dup = NULL; + switch_event_header_t *hp; + switch_status_t status; + int n; + + /* (1) well-formed EF_UNIQ source -> unique dup */ + status = switch_event_create(&src, SWITCH_EVENT_CHANNEL_DATA); + fst_xcheck(status == SWITCH_STATUS_SUCCESS, "create CHANNEL_DATA"); + fst_xcheck(switch_test_flag(src, EF_UNIQ_HEADERS) != 0, "CHANNEL_DATA is EF_UNIQ"); + switch_event_add_header_string(src, SWITCH_STACK_BOTTOM, "k", "v1"); + switch_event_add_header_string(src, SWITCH_STACK_BOTTOM, "k", "v2"); + n = 0; + for (hp = src->headers; hp; hp = hp->next) { + if (!strcmp(hp->name, "k")) { + n++; + } + } + fst_xcheck(n == 1, "EF_UNIQ source keeps 'k' unique (collapsed to last)"); + status = switch_event_dup(&dup, src); + fst_xcheck(status == SWITCH_STATUS_SUCCESS, "dup ok (unique)"); + n = 0; + for (hp = dup->headers; hp; hp = hp->next) { + if (!strcmp(hp->name, "k")) { + n++; + } + } + fst_xcheck(n == 1, "dup of unique source stays unique"); + fst_check_string_equals(switch_event_get_header(dup, "k"), "v2"); + switch_event_destroy(&dup); + switch_event_destroy(&src); + + /* (2) malformed source (duplicate names under EF_UNIQ) -> faithful copy */ + status = switch_event_create(&src, SWITCH_EVENT_GENERAL); + fst_xcheck(status == SWITCH_STATUS_SUCCESS, "create GENERAL"); + fst_xcheck(switch_test_flag(src, EF_UNIQ_HEADERS) == 0, "GENERAL is not EF_UNIQ"); + switch_event_add_header_string(src, SWITCH_STACK_BOTTOM, "dupkey", "first"); + switch_event_add_header_string(src, SWITCH_STACK_BOTTOM, "dupkey", "second"); + switch_set_flag(src, EF_UNIQ_HEADERS); + n = 0; + for (hp = src->headers; hp; hp = hp->next) { + if (!strcmp(hp->name, "dupkey")) { + n++; + } + } + fst_xcheck(n == 2, "malformed source holds 2 'dupkey' headers"); + status = switch_event_dup(&dup, src); + fst_xcheck(status == SWITCH_STATUS_SUCCESS, "dup ok (malformed)"); + fst_xcheck(switch_test_flag(dup, EF_UNIQ_HEADERS) != 0, "dup keeps EF_UNIQ flag"); + n = 0; + for (hp = dup->headers; hp; hp = hp->next) { + if (!strcmp(hp->name, "dupkey")) { + n++; + } + } + fst_xcheck(n == 2, "dup faithfully preserves both 'dupkey' headers (no silent collapse)"); + switch_event_destroy(&dup); + switch_event_destroy(&src); +} +FST_TEST_END() + FST_SUITE_END() FST_MINCORE_END()