Fix O(N²) entity addition in CallbackGroup (#2942)#3166
Open
PavelGuzenfeld wants to merge 3 commits into
Open
Conversation
Move expired weak_ptr cleanup from add_* methods (O(N) per call, O(N²) total when adding N entities) into collect_all_ptrs via a new collect_and_compact helper. collect_all_ptrs already iterates all entries on every executor spin, so cleanup happens at zero extra cost. The add_* methods become O(1) (just push_back). This fixes the quadratic slowdown reported in ros2#2942 (429ms → 6ms for 10,000 timers). This change is safe because: - collect_all_ptrs already skipped expired entries; now it also removes them in the same pass - The executor calls collect_all_ptrs on every spin iteration, so expired entries are cleaned up promptly - All find_*_ptrs_if methods already handle expired weak_ptrs gracefully Fixes ros2#2942 Signed-off-by: Pavel Guzenfeld <pavelguzenfeld@gmail.com> Generated-by: Claude Opus 4.6 (Anthropic)
Tests verify that: - Expired weak_ptrs are compacted during collect_all_ptrs - collect_all_ptrs only yields live entities - Adding N timers is not quadratic (5000 timers in < 5s) - Interleaved add/remove cycles produce correct entity counts - Mixed entity types (timers + subscriptions) are cleaned independently Signed-off-by: Pavel Guzenfeld <pavelguzenfeld@gmail.com> Generated-by: Claude Opus 4.6 (Anthropic)
- Remove mutable from entity vectors, make collect_all_ptrs non-const - Replace collect_and_compact helper with std::remove_if (erase-remove idiom) - Remove leaked test_time_source_deadlock CMake entry from another branch - Fix uncrustify formatting in test file Signed-off-by: Pavel Guzenfeld <pavelguzenfeld@gmail.com> Generated-by: Claude Opus 4.6 (Anthropic)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2942
The
add_*methods (add_timer,add_subscription,add_service,add_client,add_waitable) scanned the whole entity list to drop expiredweak_ptrs on every call, so adding N entities was O(N²). In the #2942 reproducer (10,000 timers) that scan is ~96% of the wall time.This moves the expired-entry cleanup out of the
add_*methods — which become a plainpush_back— and intocollect_all_ptrs, which already walks every entry on each executor spin and so can compact in the same pass at no extra cost (erase/remove_if).collect_all_ptrsis now non-const; there's no public API or ABI change.It's safe because
collect_all_ptrsalready skipped expired entries and now also removes them, the executor calls it every spin so cleanup stays prompt, and thefind_*_ptrs_ifhelpers already tolerate expiredweak_ptrs.Reproducer from #2942 (10,000 timers): 429 ms → 6 ms.
Adds
test_callback_group_entity_cleanup.cppcovering compaction duringcollect_all_ptrs, live-only collection, non-quadratic insertion, interleaved add/remove, and mixed entity types.Some of this change was produced with Claude Opus 4.6 (Anthropic).