From fa1554cac04c694b7b0a1b7110e02e48ff5c91cb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 7 Feb 2026 09:54:43 +0100 Subject: [PATCH] [scheduler] Eliminate heap allocation in full_cleanup_removed_items_ Replace the temporary std::vector copy with in-place compaction using a read/write pointer pattern. This avoids a heap allocation+deallocation cycle during scheduler cleanup, reducing heap fragmentation on long-running ESP devices. The new approach compacts valid items forward in the existing vector, recycles removed items as they are encountered, then resizes the vector (no reallocation since size only shrinks). Same O(n) complexity, same behavior, zero allocations. --- esphome/core/scheduler.cpp | 21 +- tests/unit_tests/test_scheduler_cleanup.cpp | 433 ++++++++++++++++++++ 2 files changed, 443 insertions(+), 11 deletions(-) create mode 100644 tests/unit_tests/test_scheduler_cleanup.cpp diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 047bf4ef17..047ad02a8c 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -390,20 +390,19 @@ void Scheduler::full_cleanup_removed_items_() { // 4. No operations inside can block or take other locks, so no deadlock risk LockGuard guard{this->lock_}; - std::vector> valid_items; - - // Move all non-removed items to valid_items, recycle removed ones - for (auto &item : this->items_) { - if (!is_item_removed_(item.get())) { - valid_items.push_back(std::move(item)); + // Compact in-place: move valid items forward, recycle removed ones + size_t write = 0; + for (size_t read = 0; read < this->items_.size(); ++read) { + if (!is_item_removed_(this->items_[read].get())) { + if (write != read) { + this->items_[write] = std::move(this->items_[read]); + } + ++write; } else { - // Recycle removed items - this->recycle_item_main_loop_(std::move(item)); + this->recycle_item_main_loop_(std::move(this->items_[read])); } } - - // Replace items_ with the filtered list - this->items_ = std::move(valid_items); + this->items_.resize(write); // Rebuild the heap structure since items are no longer in heap order std::make_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); this->to_remove_ = 0; diff --git a/tests/unit_tests/test_scheduler_cleanup.cpp b/tests/unit_tests/test_scheduler_cleanup.cpp new file mode 100644 index 0000000000..2b727697ee --- /dev/null +++ b/tests/unit_tests/test_scheduler_cleanup.cpp @@ -0,0 +1,433 @@ +// Test that in-place compaction in full_cleanup_removed_items_() produces +// identical results to the old vector-copy approach. +// Compile with: g++ -std=gnu++20 -o test_scheduler_cleanup test_scheduler_cleanup.cpp && ./test_scheduler_cleanup + +#include +#include +#include +#include +#include +#include + +// Minimal stand-in for SchedulerItem — only the fields the cleanup touches. +struct Item { + uint32_t id; // identifies the item for verification + bool remove; // mirrors SchedulerItem::remove + uint64_t next_execution; // for heap ordering + + Item(uint32_t id, bool remove, uint64_t next_execution) : id(id), remove(remove), next_execution(next_execution) {} +}; + +// Comparator matching SchedulerItem::cmp (min-heap by next_execution) +struct ItemCmp { + bool operator()(const std::unique_ptr &a, const std::unique_ptr &b) const { + return a->next_execution > b->next_execution; + } +}; + +// --- Old implementation (vector copy) ---------------------------------------- +struct OldResult { + std::vector kept_ids; // ids of items kept, in heap order + std::vector recycled_ids; // ids of items recycled +}; + +OldResult run_old(std::vector> &items) { + OldResult result; + + std::vector> valid_items; + for (auto &item : items) { + if (!item->remove) { + valid_items.push_back(std::move(item)); + } else { + result.recycled_ids.push_back(item->id); + } + } + items = std::move(valid_items); + std::make_heap(items.begin(), items.end(), ItemCmp{}); + + // Extract sorted order by popping the heap + while (!items.empty()) { + std::pop_heap(items.begin(), items.end(), ItemCmp{}); + result.kept_ids.push_back(items.back()->id); + items.pop_back(); + } + return result; +} + +// --- New implementation (in-place compaction) --------------------------------- +struct NewResult { + std::vector kept_ids; + std::vector recycled_ids; +}; + +NewResult run_new(std::vector> &items) { + NewResult result; + + size_t write = 0; + for (size_t read = 0; read < items.size(); ++read) { + if (!items[read]->remove) { + if (write != read) { + items[write] = std::move(items[read]); + } + ++write; + } else { + result.recycled_ids.push_back(items[read]->id); + } + } + items.resize(write); + std::make_heap(items.begin(), items.end(), ItemCmp{}); + + while (!items.empty()) { + std::pop_heap(items.begin(), items.end(), ItemCmp{}); + result.kept_ids.push_back(items.back()->id); + items.pop_back(); + } + return result; +} + +// --- Helpers ----------------------------------------------------------------- + +// Build a fresh item list from {id, remove, next_execution} triples. +using Spec = std::tuple; + +std::vector> build(const std::vector &specs) { + std::vector> items; + for (auto &[id, rm, exec] : specs) { + items.push_back(std::make_unique(id, rm, exec)); + } + // Put into heap order like the real scheduler + std::make_heap(items.begin(), items.end(), ItemCmp{}); + return items; +} + +void assert_equal(const OldResult &old_r, const NewResult &new_r) { + assert(old_r.kept_ids == new_r.kept_ids); + + // Recycled order may differ (old iterates linearly, new also iterates linearly, + // but heap order changes iteration order). Sort before comparing. + auto old_recycled = old_r.recycled_ids; + auto new_recycled = new_r.recycled_ids; + std::sort(old_recycled.begin(), old_recycled.end()); + std::sort(new_recycled.begin(), new_recycled.end()); + assert(old_recycled == new_recycled); +} + +// Test harness +#define TEST(name) static void test_##name() +#define RUN_TEST(name) \ + do { \ + printf(" " #name "..."); \ + test_##name(); \ + printf(" OK\n"); \ + } while (0) + +// ============================================================================= +// Tests +// ============================================================================= + +TEST(empty_list) { + auto items_old = build({}); + auto items_new = build({}); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.empty()); + assert(old_r.recycled_ids.empty()); +} + +TEST(no_removals) { + std::vector specs = { + {1, false, 100}, + {2, false, 200}, + {3, false, 300}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.size() == 3); + assert(old_r.recycled_ids.empty()); +} + +TEST(all_removed) { + std::vector specs = { + {1, true, 100}, + {2, true, 200}, + {3, true, 300}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.empty()); + assert(old_r.recycled_ids.size() == 3); +} + +TEST(single_item_kept) { + std::vector specs = {{42, false, 500}}; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.size() == 1); + assert(old_r.kept_ids[0] == 42); +} + +TEST(single_item_removed) { + std::vector specs = {{42, true, 500}}; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.empty()); + assert(old_r.recycled_ids.size() == 1); +} + +TEST(remove_first_only) { + std::vector specs = { + {1, true, 100}, + {2, false, 200}, + {3, false, 300}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.size() == 2); + assert(old_r.recycled_ids.size() == 1); +} + +TEST(remove_last_only) { + std::vector specs = { + {1, false, 100}, + {2, false, 200}, + {3, true, 300}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.size() == 2); + assert(old_r.recycled_ids.size() == 1); +} + +TEST(remove_middle_only) { + std::vector specs = { + {1, false, 100}, + {2, true, 200}, + {3, false, 300}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); +} + +TEST(alternating_remove_keep) { + std::vector specs = { + {1, true, 100}, {2, false, 200}, {3, true, 300}, {4, false, 400}, {5, true, 500}, {6, false, 600}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.size() == 3); + assert(old_r.recycled_ids.size() == 3); +} + +TEST(alternating_keep_remove) { + std::vector specs = { + {1, false, 100}, {2, true, 200}, {3, false, 300}, {4, true, 400}, {5, false, 500}, {6, true, 600}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); +} + +TEST(consecutive_removals_at_start) { + std::vector specs = { + {1, true, 100}, {2, true, 200}, {3, true, 300}, {4, false, 400}, {5, false, 500}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); +} + +TEST(consecutive_removals_at_end) { + std::vector specs = { + {1, false, 100}, {2, false, 200}, {3, true, 300}, {4, true, 400}, {5, true, 500}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); +} + +TEST(duplicate_execution_times) { + // Multiple items with the same next_execution — heap order is implementation-defined + // but both algorithms must produce the same set of kept/recycled items + std::vector specs = { + {1, false, 100}, {2, true, 100}, {3, false, 100}, {4, true, 200}, {5, false, 200}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + + // Same sets (sorted), even if pop order differs for equal timestamps + auto old_kept_sorted = old_r.kept_ids; + auto new_kept_sorted = new_r.kept_ids; + std::sort(old_kept_sorted.begin(), old_kept_sorted.end()); + std::sort(new_kept_sorted.begin(), new_kept_sorted.end()); + assert(old_kept_sorted == new_kept_sorted); + + auto old_recycled = old_r.recycled_ids; + auto new_recycled = new_r.recycled_ids; + std::sort(old_recycled.begin(), old_recycled.end()); + std::sort(new_recycled.begin(), new_recycled.end()); + assert(old_recycled == new_recycled); +} + +TEST(large_list_sparse_removals) { + // 100 items, every 10th removed + std::vector specs; + for (uint32_t i = 0; i < 100; i++) { + specs.push_back({i, (i % 10 == 0), i * 100}); + } + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.size() == 90); + assert(old_r.recycled_ids.size() == 10); +} + +TEST(large_list_dense_removals) { + // 100 items, only every 10th kept + std::vector specs; + for (uint32_t i = 0; i < 100; i++) { + specs.push_back({i, (i % 10 != 0), i * 100}); + } + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.size() == 10); + assert(old_r.recycled_ids.size() == 90); +} + +TEST(realistic_scheduler_25_cancel) { + // Mimics the integration test: 25 cancelled + 5 active post-cleanup timeouts + std::vector specs; + for (uint32_t i = 0; i < 25; i++) { + specs.push_back({i, true, 2500}); // cancelled timeouts + } + for (uint32_t i = 25; i < 30; i++) { + specs.push_back({i, false, 50 + (i - 25) * 25}); // post-cleanup timeouts + } + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); + assert(old_r.kept_ids.size() == 5); + assert(old_r.recycled_ids.size() == 25); +} + +TEST(heap_order_preserved) { + // Verify the kept items come out in correct min-heap order (ascending next_execution) + std::vector specs = { + {1, false, 500}, {2, true, 100}, {3, false, 300}, {4, true, 700}, + {5, false, 100}, {6, false, 900}, {7, true, 200}, {8, false, 400}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + + // Both should produce items in ascending execution time order + // (since we pop from min-heap) + assert(old_r.kept_ids == new_r.kept_ids); +} + +TEST(two_items_first_removed) { + std::vector specs = { + {1, true, 100}, + {2, false, 200}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); +} + +TEST(two_items_second_removed) { + std::vector specs = { + {1, false, 100}, + {2, true, 200}, + }; + auto items_old = build(specs); + auto items_new = build(specs); + auto old_r = run_old(items_old); + auto new_r = run_new(items_new); + assert_equal(old_r, new_r); +} + +// ============================================================================= +// Main +// ============================================================================= + +int main() { + printf("Scheduler cleanup equivalence tests\n"); + printf("====================================\n\n"); + + printf("Basic cases:\n"); + RUN_TEST(empty_list); + RUN_TEST(no_removals); + RUN_TEST(all_removed); + RUN_TEST(single_item_kept); + RUN_TEST(single_item_removed); + + printf("\nEdge removal positions:\n"); + RUN_TEST(remove_first_only); + RUN_TEST(remove_last_only); + RUN_TEST(remove_middle_only); + RUN_TEST(two_items_first_removed); + RUN_TEST(two_items_second_removed); + + printf("\nPatterns:\n"); + RUN_TEST(alternating_remove_keep); + RUN_TEST(alternating_keep_remove); + RUN_TEST(consecutive_removals_at_start); + RUN_TEST(consecutive_removals_at_end); + RUN_TEST(duplicate_execution_times); + + printf("\nScale:\n"); + RUN_TEST(large_list_sparse_removals); + RUN_TEST(large_list_dense_removals); + RUN_TEST(realistic_scheduler_25_cancel); + + printf("\nHeap correctness:\n"); + RUN_TEST(heap_order_preserved); + + printf("\n====================================\n"); + printf("All tests passed!\n"); + + return 0; +}