From 85435e6b5f695b37a0efd688c1848373762f191e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 28 Jul 2025 10:54:16 -1000 Subject: [PATCH] [scheduler] Eliminate more runtime string allocations from retry (#9930) --- esphome/core/scheduler.cpp | 61 ++++++++++++------ esphome/core/scheduler.h | 32 +++++++--- .../fixtures/scheduler_retry_test.yaml | 62 ++++++++++++++++++ .../integration/test_scheduler_retry_test.py | 64 ++++++++++++++++++- 4 files changed, 192 insertions(+), 27 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index a2c16c41fb..6269a66543 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -83,6 +83,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type item->type = type; item->callback = std::move(func); item->remove = false; + item->is_retry = is_retry; #ifndef ESPHOME_THREAD_SINGLE // Special handling for defer() (delay = 0, type = TIMEOUT) @@ -134,8 +135,8 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // For retries, check if there's a cancelled timeout first if (is_retry && name_cstr != nullptr && type == SchedulerItem::TIMEOUT && - (has_cancelled_timeout_in_container_(this->items_, component, name_cstr) || - has_cancelled_timeout_in_container_(this->to_add_, component, name_cstr))) { + (has_cancelled_timeout_in_container_(this->items_, component, name_cstr, /* match_retry= */ true) || + has_cancelled_timeout_in_container_(this->to_add_, component, name_cstr, /* match_retry= */ true))) { // Skip scheduling - the retry was cancelled #ifdef ESPHOME_DEBUG_SCHEDULER ESP_LOGD(TAG, "Skipping retry '%s' - found cancelled item", name_cstr); @@ -198,25 +199,27 @@ void retry_handler(const std::shared_ptr &args) { // second execution of `func` happens after `initial_wait_time` args->scheduler->set_timer_common_( args->component, Scheduler::SchedulerItem::TIMEOUT, false, &args->name, args->current_interval, - [args]() { retry_handler(args); }, true); + [args]() { retry_handler(args); }, /* is_retry= */ true); // backoff_increase_factor applied to third & later executions args->current_interval *= args->backoff_increase_factor; } -void HOT Scheduler::set_retry(Component *component, const std::string &name, uint32_t initial_wait_time, - uint8_t max_attempts, std::function func, - float backoff_increase_factor) { - if (!name.empty()) - this->cancel_retry(component, name); +void HOT Scheduler::set_retry_common_(Component *component, bool is_static_string, const void *name_ptr, + uint32_t initial_wait_time, uint8_t max_attempts, + std::function func, float backoff_increase_factor) { + const char *name_cstr = this->get_name_cstr_(is_static_string, name_ptr); + + if (name_cstr != nullptr) + this->cancel_retry(component, name_cstr); if (initial_wait_time == SCHEDULER_DONT_RUN) return; ESP_LOGVV(TAG, "set_retry(name='%s', initial_wait_time=%" PRIu32 ", max_attempts=%u, backoff_factor=%0.1f)", - name.c_str(), initial_wait_time, max_attempts, backoff_increase_factor); + name_cstr ? name_cstr : "", initial_wait_time, max_attempts, backoff_increase_factor); if (backoff_increase_factor < 0.0001) { - ESP_LOGE(TAG, "backoff_factor %0.1f too small, using 1.0: %s", backoff_increase_factor, name.c_str()); + ESP_LOGE(TAG, "backoff_factor %0.1f too small, using 1.0: %s", backoff_increase_factor, name_cstr ? name_cstr : ""); backoff_increase_factor = 1; } @@ -225,15 +228,36 @@ void HOT Scheduler::set_retry(Component *component, const std::string &name, uin args->retry_countdown = max_attempts; args->current_interval = initial_wait_time; args->component = component; - args->name = "retry$" + name; + args->name = name_cstr ? name_cstr : ""; // Convert to std::string for RetryArgs args->backoff_increase_factor = backoff_increase_factor; args->scheduler = this; - // First execution of `func` immediately - this->set_timeout(component, args->name, 0, [args]() { retry_handler(args); }); + // First execution of `func` immediately - use set_timer_common_ with is_retry=true + this->set_timer_common_( + component, SchedulerItem::TIMEOUT, false, &args->name, 0, [args]() { retry_handler(args); }, + /* is_retry= */ true); +} + +void HOT Scheduler::set_retry(Component *component, const std::string &name, uint32_t initial_wait_time, + uint8_t max_attempts, std::function func, + float backoff_increase_factor) { + this->set_retry_common_(component, false, &name, initial_wait_time, max_attempts, std::move(func), + backoff_increase_factor); +} + +void HOT Scheduler::set_retry(Component *component, const char *name, uint32_t initial_wait_time, uint8_t max_attempts, + std::function func, float backoff_increase_factor) { + this->set_retry_common_(component, true, name, initial_wait_time, max_attempts, std::move(func), + backoff_increase_factor); } bool HOT Scheduler::cancel_retry(Component *component, const std::string &name) { - return this->cancel_timeout(component, "retry$" + name); + return this->cancel_retry(component, name.c_str()); +} + +bool HOT Scheduler::cancel_retry(Component *component, const char *name) { + // Cancel timeouts that have is_retry flag set + LockGuard guard{this->lock_}; + return this->cancel_item_locked_(component, name, SchedulerItem::TIMEOUT, /* match_retry= */ true); } optional HOT Scheduler::next_schedule_in(uint32_t now) { @@ -479,7 +503,8 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co } // Helper to cancel items by name - must be called with lock held -bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { +bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type, + bool match_retry) { // Early return if name is invalid - no items to cancel if (name_cstr == nullptr) { return false; @@ -492,7 +517,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Only check defer queue for timeouts (intervals never go there) if (type == SchedulerItem::TIMEOUT) { for (auto &item : this->defer_queue_) { - if (this->matches_item_(item, component, name_cstr, type)) { + if (this->matches_item_(item, component, name_cstr, type, match_retry)) { item->remove = true; total_cancelled++; } @@ -502,7 +527,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Cancel items in the main heap for (auto &item : this->items_) { - if (this->matches_item_(item, component, name_cstr, type)) { + if (this->matches_item_(item, component, name_cstr, type, match_retry)) { item->remove = true; total_cancelled++; this->to_remove_++; // Track removals for heap items @@ -511,7 +536,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Cancel items in to_add_ for (auto &item : this->to_add_) { - if (this->matches_item_(item, component, name_cstr, type)) { + if (this->matches_item_(item, component, name_cstr, type, match_retry)) { item->remove = true; total_cancelled++; // Don't track removals for to_add_ items diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index fa189bacf7..a6092e1b1e 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -61,7 +61,10 @@ class Scheduler { bool cancel_interval(Component *component, const char *name); void set_retry(Component *component, const std::string &name, uint32_t initial_wait_time, uint8_t max_attempts, std::function func, float backoff_increase_factor = 1.0f); + void set_retry(Component *component, const char *name, uint32_t initial_wait_time, uint8_t max_attempts, + std::function func, float backoff_increase_factor = 1.0f); bool cancel_retry(Component *component, const std::string &name); + bool cancel_retry(Component *component, const char *name); // Calculate when the next scheduled item should run // @param now Fresh timestamp from millis() - must not be stale/cached @@ -98,11 +101,18 @@ class Scheduler { enum Type : uint8_t { TIMEOUT, INTERVAL } type : 1; bool remove : 1; bool name_is_dynamic : 1; // True if name was dynamically allocated (needs delete[]) - // 5 bits padding + bool is_retry : 1; // True if this is a retry timeout + // 4 bits padding // Constructor SchedulerItem() - : component(nullptr), interval(0), next_execution_(0), type(TIMEOUT), remove(false), name_is_dynamic(false) { + : component(nullptr), + interval(0), + next_execution_(0), + type(TIMEOUT), + remove(false), + name_is_dynamic(false), + is_retry(false) { name_.static_name = nullptr; } @@ -156,6 +166,10 @@ class Scheduler { void set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, uint32_t delay, std::function func, bool is_retry = false); + // Common implementation for retry + void set_retry_common_(Component *component, bool is_static_string, const void *name_ptr, uint32_t initial_wait_time, + uint8_t max_attempts, std::function func, float backoff_increase_factor); + uint64_t millis_64_(uint32_t now); // Cleanup logically deleted items from the scheduler // Returns the number of items remaining after cleanup @@ -165,7 +179,7 @@ class Scheduler { private: // Helper to cancel items by name - must be called with lock held - bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); + bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type, bool match_retry = false); // Helper to extract name as const char* from either static string or std::string inline const char *get_name_cstr_(bool is_static_string, const void *name_ptr) { @@ -177,8 +191,9 @@ class Scheduler { // Helper function to check if item matches criteria for cancellation inline bool HOT matches_item_(const std::unique_ptr &item, Component *component, const char *name_cstr, - SchedulerItem::Type type, bool skip_removed = true) const { - if (item->component != component || item->type != type || (skip_removed && item->remove)) { + SchedulerItem::Type type, bool match_retry, bool skip_removed = true) const { + if (item->component != component || item->type != type || (skip_removed && item->remove) || + (match_retry && !item->is_retry)) { return false; } const char *item_name = item->get_name(); @@ -206,10 +221,11 @@ class Scheduler { // Template helper to check if any item in a container matches our criteria template - bool has_cancelled_timeout_in_container_(const Container &container, Component *component, - const char *name_cstr) const { + bool has_cancelled_timeout_in_container_(const Container &container, Component *component, const char *name_cstr, + bool match_retry) const { for (const auto &item : container) { - if (item->remove && this->matches_item_(item, component, name_cstr, SchedulerItem::TIMEOUT, false)) { + if (item->remove && this->matches_item_(item, component, name_cstr, SchedulerItem::TIMEOUT, match_retry, + /* skip_removed= */ false)) { return true; } } diff --git a/tests/integration/fixtures/scheduler_retry_test.yaml b/tests/integration/fixtures/scheduler_retry_test.yaml index c6fcc53f8c..11fff6c395 100644 --- a/tests/integration/fixtures/scheduler_retry_test.yaml +++ b/tests/integration/fixtures/scheduler_retry_test.yaml @@ -37,6 +37,15 @@ globals: - id: multiple_same_name_counter type: int initial_value: '0' + - id: const_char_retry_counter + type: int + initial_value: '0' + - id: static_char_retry_counter + type: int + initial_value: '0' + - id: mixed_cancel_result + type: bool + initial_value: 'false' # Using different component types for each test to ensure isolation sensor: @@ -229,6 +238,56 @@ script: return RetryResult::RETRY; }); + # Test 8: Const char* overloads + - logger.log: "=== Test 8: Const char* overloads ===" + - lambda: |- + auto *component = id(simple_retry_sensor); + + // Test 8a: Direct string literal + App.scheduler.set_retry(component, "const_char_test", 30, 2, + [](uint8_t retry_countdown) { + id(const_char_retry_counter)++; + ESP_LOGI("test", "Const char retry %d", id(const_char_retry_counter)); + return RetryResult::DONE; + }); + + # Test 9: Static const char* variable + - logger.log: "=== Test 9: Static const char* ===" + - lambda: |- + auto *component = id(backoff_retry_sensor); + + static const char* STATIC_NAME = "static_retry_test"; + App.scheduler.set_retry(component, STATIC_NAME, 20, 1, + [](uint8_t retry_countdown) { + id(static_char_retry_counter)++; + ESP_LOGI("test", "Static const char retry %d", id(static_char_retry_counter)); + return RetryResult::DONE; + }); + + // Cancel with same static const char* + App.scheduler.set_timeout(component, "static_cancel", 10, []() { + static const char* STATIC_NAME = "static_retry_test"; + bool result = App.scheduler.cancel_retry(id(backoff_retry_sensor), STATIC_NAME); + ESP_LOGI("test", "Static cancel result: %s", result ? "true" : "false"); + }); + + # Test 10: Mix string and const char* cancel + - logger.log: "=== Test 10: Mixed string/const char* ===" + - lambda: |- + auto *component = id(immediate_done_sensor); + + // Set with std::string + std::string str_name = "mixed_retry"; + App.scheduler.set_retry(component, str_name, 40, 3, + [](uint8_t retry_countdown) { + ESP_LOGI("test", "Mixed retry - should be cancelled"); + return RetryResult::RETRY; + }); + + // Cancel with const char* + id(mixed_cancel_result) = App.scheduler.cancel_retry(component, "mixed_retry"); + ESP_LOGI("test", "Mixed cancel result: %s", id(mixed_cancel_result) ? "true" : "false"); + # Wait for all tests to complete before reporting - delay: 500ms @@ -242,4 +301,7 @@ script: ESP_LOGI("test", "Empty name retry counter: %d (expected 1-2)", id(empty_name_retry_counter)); ESP_LOGI("test", "Component retry counter: %d (expected 2)", id(script_retry_counter)); ESP_LOGI("test", "Multiple same name counter: %d (expected 20+)", id(multiple_same_name_counter)); + ESP_LOGI("test", "Const char retry counter: %d (expected 1)", id(const_char_retry_counter)); + ESP_LOGI("test", "Static char retry counter: %d (expected 1)", id(static_char_retry_counter)); + ESP_LOGI("test", "Mixed cancel result: %s (expected true)", id(mixed_cancel_result) ? "true" : "false"); ESP_LOGI("test", "All retry tests completed"); diff --git a/tests/integration/test_scheduler_retry_test.py b/tests/integration/test_scheduler_retry_test.py index 1a469fcff1..c04b7197c9 100644 --- a/tests/integration/test_scheduler_retry_test.py +++ b/tests/integration/test_scheduler_retry_test.py @@ -23,6 +23,9 @@ async def test_scheduler_retry_test( empty_name_retry_done = asyncio.Event() component_retry_done = asyncio.Event() multiple_name_done = asyncio.Event() + const_char_done = asyncio.Event() + static_char_done = asyncio.Event() + mixed_cancel_done = asyncio.Event() test_complete = asyncio.Event() # Track retry counts @@ -33,16 +36,20 @@ async def test_scheduler_retry_test( empty_name_retry_count = 0 component_retry_count = 0 multiple_name_count = 0 + const_char_retry_count = 0 + static_char_retry_count = 0 # Track specific test results cancel_result = None empty_cancel_result = None + mixed_cancel_result = None backoff_intervals = [] def on_log_line(line: str) -> None: nonlocal simple_retry_count, backoff_retry_count, immediate_done_count nonlocal cancel_retry_count, empty_name_retry_count, component_retry_count - nonlocal multiple_name_count, cancel_result, empty_cancel_result + nonlocal multiple_name_count, const_char_retry_count, static_char_retry_count + nonlocal cancel_result, empty_cancel_result, mixed_cancel_result # Strip ANSI color codes clean_line = re.sub(r"\x1b\[[0-9;]*m", "", line) @@ -106,6 +113,27 @@ async def test_scheduler_retry_test( if multiple_name_count >= 20: multiple_name_done.set() + # Const char retry test + elif "Const char retry" in clean_line: + if match := re.search(r"Const char retry (\d+)", clean_line): + const_char_retry_count = int(match.group(1)) + const_char_done.set() + + # Static const char retry test + elif "Static const char retry" in clean_line: + if match := re.search(r"Static const char retry (\d+)", clean_line): + static_char_retry_count = int(match.group(1)) + static_char_done.set() + + elif "Static cancel result:" in clean_line: + # This is part of test 9, but we don't track it separately + pass + + # Mixed cancel test + elif "Mixed cancel result:" in clean_line: + mixed_cancel_result = "true" in clean_line + mixed_cancel_done.set() + # Test completion elif "All retry tests completed" in clean_line: test_complete.set() @@ -227,6 +255,40 @@ async def test_scheduler_retry_test( f"Expected multiple name count >= 20 (second retry only), got {multiple_name_count}" ) + # Wait for const char retry test + try: + await asyncio.wait_for(const_char_done.wait(), timeout=1.0) + except TimeoutError: + pytest.fail( + f"Const char retry test did not complete. Count: {const_char_retry_count}" + ) + + assert const_char_retry_count == 1, ( + f"Expected 1 const char retry call, got {const_char_retry_count}" + ) + + # Wait for static char retry test + try: + await asyncio.wait_for(static_char_done.wait(), timeout=1.0) + except TimeoutError: + pytest.fail( + f"Static char retry test did not complete. Count: {static_char_retry_count}" + ) + + assert static_char_retry_count == 1, ( + f"Expected 1 static char retry call, got {static_char_retry_count}" + ) + + # Wait for mixed cancel test + try: + await asyncio.wait_for(mixed_cancel_done.wait(), timeout=1.0) + except TimeoutError: + pytest.fail("Mixed cancel test did not complete") + + assert mixed_cancel_result is True, ( + "Mixed string/const char cancel should have succeeded" + ) + # Wait for test completion try: await asyncio.wait_for(test_complete.wait(), timeout=1.0)