Optimize timer service for lock-free schedule/cancel#112
Conversation
Replace heap-allocated timer_op with an embedded completion_op in timer_impl, eliminating new/delete per fire and cancel. Defer heap insertion from expires_after() to wait() so idle timers never touch the mutex. Add thread-local caches for both timer_impl recycling and service lookup to avoid locking on the common single-thread path. Cache the nearest expiry in an atomic to make nearest_expiry() and empty() lock-free. Add might_have_pending_waits_ flag so cancel returns immediately when no wait was ever issued. With all fast paths hit, the schedule/cancel cycle takes zero mutex locks. Already-expired timers (e.g. 0ns delay) short-circuit in wait() by posting directly to the executor, bypassing the heap and epoll entirely.
📝 WalkthroughWalkthroughThe timer service implementation is refactored to embed a completion operation within timer_impl, replacing external timer_op handling. Thread-local caching mechanisms are introduced to reduce allocations and improve performance. The service data structures are reorganized from intrusive lists to free-list pointers with live counting and lock-free cache queries. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Cache as Thread-Local<br/>Cache
participant TimerImpl as timer_impl
participant Service as timer_service_impl
participant Executor as Executor/<br/>Scheduler
rect rgba(0, 128, 255, 0.5)
Note over Client,Executor: Timer Creation with Caching
Client->>Cache: try_pop_tl_cache()
alt Cache Hit
Cache-->>Client: timer_impl*
else Cache Miss
Client->>Service: create_impl()
Service->>TimerImpl: allocate & initialize
TimerImpl->>TimerImpl: embed completion_op
Service-->>Client: timer_impl*
end
end
rect rgba(34, 139, 34, 0.5)
Note over Client,Executor: Timer Expiry & Completion
Service->>Service: process_expired()
Service->>Executor: post(&impl.op_)
Executor->>TimerImpl: invoke completion_op
TimerImpl->>Executor: post coroutine handle
Executor->>Executor: signal work complete
end
rect rgba(255, 140, 0, 0.5)
Note over Client,Executor: Cleanup & Caching
Client->>Cache: try_push_tl_cache(impl)
alt Cache Available
Cache->>TimerImpl: store in thread-local
else Cache Full
Client->>Service: destroy_impl()
Service->>TimerImpl: deallocate
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
An automated preview of the documentation is available at https://112.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-09 16:03:25 UTC |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #112 +/- ##
===========================================
+ Coverage 80.42% 80.47% +0.05%
===========================================
Files 65 65
Lines 5594 5630 +36
===========================================
+ Hits 4499 4531 +32
- Misses 1095 1099 +4
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/timer_service.cpp`:
- Around line 200-234: The thread-local cache deletions never decrement
live_count_, causing a leak: update timer_service_invalidate_cache() so that
when it removes and deletes cached impls it also decrements the owning
TimerService's live_count_ (use the service pointer available in the impl to do
--live_count_ or call a new TimerService::release_impl/count_decrement helper)
before calling delete; in shutdown() ensure that when you call
timer_service_invalidate_cache() you also attempt to invalidate other threads'
caches where possible and that the code path which deletes impls from heap_ or
free_list_ consistently updates live_count_ (heap_ loop and free-list draining
should keep the existing --live_count_ behavior), and for ~tl_impl_cache
document that at thread-exit the service may be gone so raw delete without
decrement is unavoidable. Ensure references to timer_service_invalidate_cache(),
~tl_impl_cache, destroy_impl, live_count_, shutdown(), heap_, and free_list_ are
used to locate the changes.
🧹 Nitpick comments (4)
src/corosio/src/detail/timer_service.cpp (4)
344-355: Non-atomic reads ofheap_index_andwaiting_are technically data races (UB).Lines 346 and 350–351 read
might_have_pending_waits_,heap_index_, andwaiting_without synchronization. Meanwhile,process_expired()writesheap_index_andwaiting_under the mutex on the scheduler thread. Although the fast-path checks are conservatively correct (a stale value always falls through to the locked path), unsynchronized concurrent reads/writes of non-atomic variables are undefined behavior under the C++ memory model.The same pattern exists in
update_timer()(line 285–288) anddestroy_impl()(line 266).Consider making
heap_index_,waiting_, andmight_have_pending_waits_std::atomicwith relaxed loads on the fast path and release stores under the lock — or adding a brief comment justifying the data race as intentional on the target platforms.
264-280:destroy_implcan acquire the mutex twice for a heap-resident timer.When
heap_index_ != max, the mutex is acquired to remove the timer from the heap (line 268), released, and then acquired again to push onto the free-list (line 276). This is functionally correct but doubles the locking cost for timers destroyed while still in the heap.Suggested consolidation
void destroy_impl(timer_impl& impl) { - if (impl.heap_index_ != (std::numeric_limits<std::size_t>::max)()) - { - std::lock_guard lock(mutex_); - remove_timer_impl(impl); - refresh_cached_nearest(); - } - - if (try_push_tl_cache(&impl)) - return; - - std::lock_guard lock(mutex_); - impl.next_free_ = free_list_; - free_list_ = &impl; - --live_count_; + bool was_in_heap = + (impl.heap_index_ != (std::numeric_limits<std::size_t>::max)()); + + if (!was_in_heap && try_push_tl_cache(&impl)) + return; + + std::lock_guard lock(mutex_); + if (impl.heap_index_ != (std::numeric_limits<std::size_t>::max)()) + { + remove_timer_impl(impl); + refresh_cached_nearest(); + } + impl.next_free_ = free_list_; + free_list_ = &impl; + --live_count_; }
569-607: Thread-local caches on other threads are not invalidated byshutdown().
timer_service_invalidate_cache()only clears the calling thread'sthread_localstate. If worker threads have cachedtimer_impl*pointers, those caches persist and the~tl_impl_cachedestructor willdeletethe impl at thread exit. Today this is safe becausetimer_impl's destructor doesn't dereferencesvc_, but it is fragile — any future addition totimer_impl's destructor that touches the service (e.g., logging, assertions, metrics) would be a use-after-free.Consider adding a brief comment near
~tl_impl_cachedocumenting this invariant ("timer_impl's destructor must never accesssvc_"), so future maintainers don't inadvertently break it.
385-389:nearest_expiry()assumessteady_clock::duration::repfitsint64_t.
refresh_cached_nearest()storestime_since_epoch().count()into anint64_tatomic, andnearest_expiry()reconstructs viatime_point::duration(ns). This is correct as long asduration::repisint64_t(which is the case on all major implementations), but astatic_assertwould make the assumption explicit and catch exotic platforms.static_assert( sizeof(time_point::duration::rep) <= sizeof(std::int64_t), "duration::rep must fit in int64_t for the atomic cache");
| void shutdown() override | ||
| { | ||
| // Cancel all waiting timers and destroy coroutine handles | ||
| // This properly decrements outstanding_work_ for each waiting timer | ||
| while (auto* impl = timers_.pop_front()) | ||
| timer_service_invalidate_cache(); | ||
|
|
||
| // Cancel waiting timers still in the heap | ||
| for (auto& entry : heap_) | ||
| { | ||
| auto* impl = entry.timer_; | ||
| if (impl->waiting_) | ||
| { | ||
| impl->waiting_ = false; | ||
| // Destroy the coroutine handle without resuming | ||
| impl->h_.destroy(); | ||
| // Decrement work count to avoid leak | ||
| sched_->on_work_finished(); | ||
| } | ||
| impl->heap_index_ = (std::numeric_limits<std::size_t>::max)(); | ||
| delete impl; | ||
| --live_count_; | ||
| } | ||
| while (auto* impl = free_list_.pop_front()) | ||
| delete impl; | ||
| heap_.clear(); | ||
| cached_nearest_ns_.store( | ||
| (std::numeric_limits<std::int64_t>::max)(), | ||
| std::memory_order_release); | ||
|
|
||
| // Delete free-listed impls | ||
| while (free_list_) | ||
| { | ||
| auto* next = free_list_->next_free_; | ||
| delete free_list_; | ||
| free_list_ = next; | ||
| } | ||
|
|
||
| // Any live timers not in heap and not on free list are still | ||
| // referenced by timer objects — they'll call destroy_impl() | ||
| // which will delete them (live_count_ tracks this). | ||
| } |
There was a problem hiding this comment.
live_count_ leaks when thread-local cache deletes impls.
When timer_service_invalidate_cache() (line 202/605) or ~tl_impl_cache (line 573) deletes a cached impl, live_count_ is never decremented. The impl was counted as "live" when destroy_impl pushed it to the TL cache (skipping --live_count_), but the deletion bypasses destroy_impl.
Additionally, shutdown() only calls invalidate_cache() on the calling thread. Other threads with cached impls will delete them on thread exit via ~tl_impl_cache, again without decrementing live_count_. Since live_count_ isn't used for runtime decisions today, this is cosmetic, but it would break any future assertion or diagnostic that relies on the count reaching zero.
Proposed fix in invalidate_cache
void
timer_service_invalidate_cache() noexcept
{
cached_ctx = nullptr;
cached_svc = nullptr;
- delete tl_cached_impl.ptr;
+ if (tl_cached_impl.ptr)
+ {
+ // Put it back on the free-list via destroy_impl so live_count_ stays correct
+ tl_cached_impl.ptr->svc_->destroy_impl(*tl_cached_impl.ptr);
+ }
tl_cached_impl.ptr = nullptr;
}For ~tl_impl_cache, the service may already be destroyed at thread-exit time, so a raw delete is the only safe option there — consider documenting this known leak explicitly.
🤖 Prompt for AI Agents
In `@src/corosio/src/detail/timer_service.cpp` around lines 200 - 234, The
thread-local cache deletions never decrement live_count_, causing a leak: update
timer_service_invalidate_cache() so that when it removes and deletes cached
impls it also decrements the owning TimerService's live_count_ (use the service
pointer available in the impl to do --live_count_ or call a new
TimerService::release_impl/count_decrement helper) before calling delete; in
shutdown() ensure that when you call timer_service_invalidate_cache() you also
attempt to invalidate other threads' caches where possible and that the code
path which deletes impls from heap_ or free_list_ consistently updates
live_count_ (heap_ loop and free-list draining should keep the existing
--live_count_ behavior), and for ~tl_impl_cache document that at thread-exit the
service may be gone so raw delete without decrement is unavoidable. Ensure
references to timer_service_invalidate_cache(), ~tl_impl_cache, destroy_impl,
live_count_, shutdown(), heap_, and free_list_ are used to locate the changes.
|
GCOVR code coverage report https://112.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-09 16:17:02 UTC |
2 participants
Replace heap-allocated timer_op with an embedded completion_op in timer_impl, eliminating new/delete per fire and cancel. Defer heap insertion from expires_after() to wait() so idle timers never touch the mutex. Add thread-local caches for both timer_impl recycling and service lookup to avoid locking on the common single-thread path. Cache the nearest expiry in an atomic to make nearest_expiry() and empty() lock-free. Add might_have_pending_waits_ flag so cancel returns immediately when no wait was ever issued.
With all fast paths hit, the schedule/cancel cycle takes zero mutex locks. Already-expired timers (e.g. 0ns delay) short-circuit in wait() by posting directly to the executor, bypassing the heap and epoll entirely.
Summary by CodeRabbit