Add O(1) indexed service slots to execution_context#249
Add O(1) indexed service slots to execution_context#249mvandeberg merged 1 commit intocppalliance:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA per-type slot-ID generator ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Context as execution_context
participant SlotMech as service_slot<T>
participant Slots as slots_[]
participant Mutex as Mutex+Registry
participant Factory as factory
Client->>Context: find_service<T>()
Context->>SlotMech: compute slot id
SlotMech-->>Context: slot index
Context->>Slots: atomic load slots_[index] (acquire)
alt non-null pointer
Slots-->>Context: service*
Context-->>Client: return service*
else null
Context->>Mutex: lock + lookup/create
Mutex-->>Context: service* or create via Factory
alt created or found
Context->>Slots: atomic store slots_[index] (release)
Context-->>Client: return service*
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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://249.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-04-01 21:56:48 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
include/boost/capy/ex/execution_context.hpp (1)
228-235: Don't consume cache slots on negative probes.Line 228 assigns a process-wide slot ID the first time
find_service<T>()orhas_service<T>()probesT, even when noTwill ever be registered. With Line 571 capping the cache at 32 entries, a feature-probe pass over absent services can permanently push later hot services onto the linked-list path in every fresh context. A non-allocating peek here, with slot assignment deferred until successful registration or backfill, would preserve the fast-path budget for actual services.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/execution_context.hpp` around lines 228 - 235, find_service<T>()/has_service<T>() currently calls detail::service_slot<T>() which assigns a process-wide slot on first probe and wastes cache entries; change these probe sites to use a non-allocating probe API (e.g., implement and call detail::service_slot_peek<T>() or add an allocate=false parameter to detail::service_slot<T>()) that only returns an existing slot index (or sentinel) without creating one, then keep the existing slot-index-based fast-path using slots_ and max_service_slots; defer actual slot assignment to the registration/backfill paths so only successful registrations consume slots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/capy/ex/execution_context.hpp`:
- Around line 269-292: The fast-path in use_service<T>() publishes T under its
key_type when get_key<T>::value is true but lacks the same safety sanity check
make_service performs; add a compile-time and/or runtime check in the
impl/factory path to ensure T& is convertible to key_type& before publishing
under get_key<T>::type (i.e. when get_key<T>::value is true), e.g. use
static_assert(std::is_convertible<T*, get_key<T>::type*>::value, "...") or an
equivalent SFINAE constraint in the impl constructor (the factory that uses
detail::type_id<T>() and detail::type_id<get_key<T>::type> and
detail::service_slot<T>), so malformed services cannot be registered and later
mis-cast by find_service<key_type>() or by a slot hit.
In `@src/ex/execution_context.cpp`:
- Around line 58-59: The teardown currently clears the linked-list registry
(head_) before nulling slots_, which can let the fast path hand out a slot
pointer being deleted; move the loop that nulls each entry in slots_ (the
for(auto& s : slots_) s.store(nullptr, std::memory_order_relaxed)) so it runs
before the delete/teardown loop and before/at the same time as head_ is made
empty, ensuring slots_ and head_ transition to empty together and preventing
re-entrancy from seeing partially-destroyed services.
---
Nitpick comments:
In `@include/boost/capy/ex/execution_context.hpp`:
- Around line 228-235: find_service<T>()/has_service<T>() currently calls
detail::service_slot<T>() which assigns a process-wide slot on first probe and
wastes cache entries; change these probe sites to use a non-allocating probe API
(e.g., implement and call detail::service_slot_peek<T>() or add an
allocate=false parameter to detail::service_slot<T>()) that only returns an
existing slot index (or sentinel) without creating one, then keep the existing
slot-index-based fast-path using slots_ and max_service_slots; defer actual slot
assignment to the registration/backfill paths so only successful registrations
consume slots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f14bded4-e961-4404-82ec-c7e1ca116d26
⛔ Files ignored due to path filters (2)
doc/unlisted/execution-contexts.adocis excluded by!**/doc/**test/unit/ex/execution_context.cppis excluded by!**/test/**
📒 Files selected for processing (3)
include/boost/capy/detail/service_slot.hppinclude/boost/capy/ex/execution_context.hppsrc/ex/execution_context.cpp
|
GCOVR code coverage report https://249.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-04-01 22:10:35 UTC |
find_service<T>() and use_service<T>() previously acquired a mutex and walked an O(n) linked list on every call. This becomes a bottleneck when services are looked up frequently at runtime (e.g. per-request timer construction). Add a fixed-size array of 32 atomic service pointers indexed by a per-type slot ID, giving lock-free O(1) reads on the fast path. The linked-list registry remains as a fallback for overflow.
mvandeberg
force-pushed
the
pr/indexed-services
branch
from
0323257 to
d00a033
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #249 +/- ##
===========================================
- Coverage 92.50% 92.46% -0.04%
===========================================
Files 170 172 +2
Lines 9348 9414 +66
===========================================
+ Hits 8647 8705 +58
- Misses 701 709 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
2 participants
find_service() and use_service() previously acquired a mutex and walked an O(n) linked list on every call. This becomes a bottleneck when services are looked up frequently at runtime (e.g. per-request timer construction). Add a fixed-size array of 32 atomic service pointers indexed by a per-type slot ID, giving lock-free O(1) reads on the fast path. The linked-list registry remains as a fallback for overflow.
Summary by CodeRabbit
Performance Improvements
Bug Fixes