refactor(worker): Use std::vector for function maps and fix static function throwing error. - #123
Closed
sitaowang1998 wants to merge 5 commits into
Closed
refactor(worker): Use std::vector for function maps and fix static function throwing error.#123sitaowang1998 wants to merge 5 commits into
std::vector for function maps and fix static function throwing error.#123sitaowang1998 wants to merge 5 commits into
Conversation
Contributor
|
Warning Rate limit exceeded@sitaowang1998 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
## Walkthrough
The changes refactor the internal storage of function registration in both `FunctionManager` and `FunctionNameManager` from hash maps to vectors of pairs, and update registration and lookup methods to use `std::string_view` for more efficient string handling. Methods that cannot throw are now marked `noexcept`. Linter suppression comments related to `cert-err58-cpp` are removed from several files, as the static initialization pattern has been refactored to avoid the need for such annotations. No functional code outside of registration and lookup mechanisms is altered.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------------|---------------|
| src/spider/worker/FunctionManager.cpp, src/spider/worker/FunctionManager.hpp | Replaced internal function map with `std::vector<std::pair<std::string_view, Function>>`, updated registration and lookup methods to use `std::string_view`, added linear search for existence checks, marked methods `noexcept`, and removed Abseil hash map dependency. |
| src/spider/worker/FunctionNameManager.cpp, src/spider/worker/FunctionNameManager.hpp | Replaced internal name map with `std::vector<std::pair<std::uintptr_t, std::string_view>>`, updated registration to use `std::string_view`, added linear search for duplicate detection, marked methods `noexcept`, and removed Abseil hash map dependency. |
| examples/quick-start/src/tasks.cpp, tests/worker/signal-test.cpp, tests/worker/worker-test.cpp | Removed `// NOLINTNEXTLINE(cert-err58-cpp)` and related linter suppression comments before or around `SPIDER_REGISTER_TASK` macro invocations. No code logic changed. |
| src/spider/core/TaskGraphImpl.hpp | Changed reinterpret_cast argument type from `void const*` to `uintptr_t` for function name lookup in `create_task` method. |
| tests/worker/test-FunctionManager.cpp | Updated test casts from `void*` to `uintptr_t` for function pointer conversions and included `<cstdint>`. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant FunctionManager
participant FunctionNameManager
User->>FunctionManager: register_function(name: string_view, func)
FunctionManager->>FunctionManager: contains(name)
alt Name not found
FunctionManager->>FunctionManager: emplace_back(name, func)
FunctionManager-->>User: true
else Name exists
FunctionManager-->>User: false
end
User->>FunctionManager: get_function(name: string_view)
FunctionManager->>FunctionManager: linear search for name
alt Found
FunctionManager-->>User: pointer to Function
else Not found
FunctionManager-->>User: nullptr
end
User->>FunctionNameManager: register_function(name: string_view, ptr)
FunctionNameManager->>FunctionNameManager: linear search for ptr
alt Ptr not found
FunctionNameManager->>FunctionNameManager: emplace_back(ptr, name)
FunctionNameManager-->>User: true
else Ptr exists
FunctionNameManager-->>User: false
endAssessment against linked issues
Possibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/spider/worker/FunctionNameManager.cpp (1)
15-18: Linear search is functional but may scale poorlyReplacing the hash lookup with a linear scan trades determinism for
O(N)look-ups.
Ifm_name_mapever grows beyond a few hundred items (e.g., >1 k tasks in a plugin), look-up latency could become noticeable at hot call-sites.-for (auto const& it : m_name_map) { - if (it.first == ptr) { - return std::string{it.second}; - } -} +if (auto it = std::ranges::find_if( + m_name_map, + [ptr](auto const& pair) { return pair.first == ptr; }); + it != m_name_map.end()) +{ + return std::string{it->second}; +}This keeps the vector but leverages
<algorithm>and short-circuits earlier.
If higher throughput is needed later, consider a parallel sorted vector +lower_bound()or a small open-addressing map instd/boostinstead.src/spider/worker/FunctionManager.hpp (2)
50-50: Vector-based map could become a scalability bottleneckSwitching from
absl::flat_hash_maptostd::vector<std::pair<…>>removes the ABI headache, but it downgrades look-up to O(n).
If the number of registered tasks grows into the hundreds/thousands (not uncommon in plugin-heavy deployments) latency on every invocation will scale linearly. Consider one of the following:
- keep the
std::vectorbut maintain it sorted and usestd::lower_bound→ O(log n);- or store an additional
std::unordered_map<std::string_view,size_t>index;- or switch to another header-only open-addressing implementation (e.g.,
robin_hood::unordered_flat_map) that does not suffer from the Abseil ABI issue.At the very least, please document the expected upper bound of registered functions so maintainers know the trade-off.
394-398: Linear search is simple but could be micro-optimised
std::ranges::any_ofprovides clarity, but if you adopt the earlier suggestion of owning a sorted vector, you can switch tostd::lower_boundfor O(log n) look-ups. No action required if function count is guaranteed small, otherwise worth considering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/quick-start/src/tasks.cpp(0 hunks)src/spider/worker/FunctionManager.cpp(2 hunks)src/spider/worker/FunctionManager.hpp(3 hunks)src/spider/worker/FunctionNameManager.cpp(1 hunks)src/spider/worker/FunctionNameManager.hpp(3 hunks)tests/worker/signal-test.cpp(0 hunks)tests/worker/worker-test.cpp(0 hunks)
💤 Files with no reviewable changes (3)
- examples/quick-start/src/tasks.cpp
- tests/worker/signal-test.cpp
- tests/worker/worker-test.cpp
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/spider/worker/FunctionNameManager.cpp (3)
src/spider/worker/FunctionManager.cpp (2)
get_instance(105-108)get_instance(105-105)src/spider/worker/DllLoader.hpp (1)
instance(12-17)src/spider/worker/FunctionNameManager.hpp (1)
ptr(54-54)
src/spider/worker/FunctionManager.cpp (4)
src/spider/worker/FunctionNameManager.cpp (2)
get_instance(9-12)get_instance(9-9)tests/worker/signal-test.cpp (2)
get_instance(11-14)get_instance(11-11)src/spider/worker/DllLoader.hpp (1)
instance(12-17)src/spider/worker/FunctionManager.hpp (4)
name(381-387)name(381-381)name(389-389)name(394-394)
src/spider/worker/FunctionNameManager.hpp (1)
src/spider/worker/FunctionManager.hpp (4)
name(381-387)name(381-381)name(389-389)name(394-394)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (2)
src/spider/worker/FunctionNameManager.cpp (1)
9-12: Singleton markednoexcept– looks goodMarking
get_instance()asnoexceptis correct; the call site can now be used in static initialisation without trippingcert-err58-cpp.src/spider/worker/FunctionManager.cpp (1)
105-108:get_instance()correctly annotatedAdding
noexceptaligns with the pattern in the other singletons and removes the need for the previous linter suppression.
Comment on lines
+110
to
+114
| auto FunctionManager::get_function(std::string_view name) const -> Function const* { | ||
| for (auto const& pair : m_function_map) { | ||
| if (pair.first == name) { | ||
| return &pair.second; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential dangling std::string_view in stored map – risk of UB
m_function_map stores std::string_view values supplied by caller code.
When register_function_invoker() is called with a temporary or a std::string that goes out of scope, the stored view will dangle.
Subsequent get_function() look-ups will then dereference invalid memory (undefined behaviour).
Suggested fix: own the key.
-using FunctionMap = std::vector<std::pair<std::string_view, Function>>;
+using FunctionMap = std::vector<std::pair<std::string, Function>>;and in the registrar:
-m_function_map.emplace_back(name, f);
+m_function_map.emplace_back(std::string{name}, f);If zero-copy is required, document that the caller must pass a string literal or other static storage and enforce it with static_assert(!std::is_same_v<std::decay_t<decltype(name)>, std::string>).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto FunctionManager::get_function(std::string_view name) const -> Function const* { | |
| for (auto const& pair : m_function_map) { | |
| if (pair.first == name) { | |
| return &pair.second; | |
| } | |
| // Change the map alias to own its keys | |
| - using FunctionMap = std::vector<std::pair<std::string_view, Function>>; | |
| + using FunctionMap = std::vector<std::pair<std::string, Function>>; | |
| // In the registrar where functions are added | |
| - m_function_map.emplace_back(name, f); | |
| + m_function_map.emplace_back(std::string{name}, f); |
Comment on lines
+50
to
+51
| m_name_map.emplace_back(reinterpret_cast<void*>(function_pointer), name); | ||
| return true; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same dangling string_view concern as in FunctionManager
name is forwarded unowned into the vector; if the argument is not a string literal the view may dangle after the call. Either:
- Change the container to hold
std::string, or - Constrain the API so only compile-time string literals are accepted (e.g., via a user-defined literal or
const char(&)[N]).
Comment on lines
364
to
379
| template <class F> | ||
| auto register_function(std::string const& name, F f) -> bool { | ||
| if (m_function_map.contains(name)) { | ||
| auto register_function(std::string_view name, F f) noexcept -> bool { | ||
| if (contains(name)) { | ||
| return false; | ||
| } | ||
| return m_function_map | ||
| .emplace( | ||
| name, | ||
| std::bind( | ||
| &FunctionInvoker<F>::apply, | ||
| std::move(f), | ||
| std::placeholders::_1, | ||
| std::placeholders::_2 | ||
| ) | ||
| m_function_map.emplace_back( | ||
| name, | ||
| std::bind( | ||
| &FunctionInvoker<F>::apply, | ||
| std::move(f), | ||
| std::placeholders::_1, | ||
| std::placeholders::_2 | ||
| ) | ||
| .second; | ||
| ); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
noexcept + std::string_view here is unsafe and may terminate the process
-
register_functionis declarednoexcept, yet:std::ranges::any_of,emplace_back,std::bind, and the allocator insidestd::vectorcan all throw (bad_alloc, etc.).- Any exception will invoke
std::terminate, hiding the root cause.
-
std::string_viewis stored in the vector.
If the caller passes anything with a shorter lifetime than static storage (e.g.,std::string namereturned from a factory), the stored view will dangle → UB.
Proposed fix:
-auto register_function(std::string_view name, F f) noexcept -> bool {
+template <class F>
+auto register_function(std::string_view name, F f) -> bool {
@@
- m_function_map.emplace_back(
- name,
+ // Own the key to avoid dangling views
+ m_function_map.emplace_back(
+ std::string{name},
std::bind(
&FunctionInvoker<F>::apply,
std::move(f),
std::placeholders::_1,
std::placeholders::_2
)
);
return true;
}(This removes noexcept and takes ownership of the key.)
Alternatively, keep the string_view but constrain the API so only string literals can be passed (harder to police).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| template <class F> | |
| auto register_function(std::string const& name, F f) -> bool { | |
| if (m_function_map.contains(name)) { | |
| auto register_function(std::string_view name, F f) noexcept -> bool { | |
| if (contains(name)) { | |
| return false; | |
| } | |
| return m_function_map | |
| .emplace( | |
| name, | |
| std::bind( | |
| &FunctionInvoker<F>::apply, | |
| std::move(f), | |
| std::placeholders::_1, | |
| std::placeholders::_2 | |
| ) | |
| m_function_map.emplace_back( | |
| name, | |
| std::bind( | |
| &FunctionInvoker<F>::apply, | |
| std::move(f), | |
| std::placeholders::_1, | |
| std::placeholders::_2 | |
| ) | |
| .second; | |
| ); | |
| return true; | |
| } | |
| template <class F> | |
| auto register_function(std::string_view name, F f) -> bool { | |
| if (contains(name)) { | |
| return false; | |
| } | |
| m_function_map.emplace_back( | |
| std::string{name}, | |
| std::bind( | |
| &FunctionInvoker<F>::apply, | |
| std::move(f), | |
| std::placeholders::_1, | |
| std::placeholders::_2 | |
| ) | |
| ); | |
| return true; | |
| } |
Comment on lines
+381
to
387
| auto register_function_invoker(std::string_view name, Function const& f) -> bool { | ||
| if (contains(name)) { | ||
| return false; | ||
| } | ||
| m_function_map.emplace_back(name, f); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same lifetime problem applies to register_function_invoker
register_function_invoker stores std::string_view without owning its data. Please apply the same remediation (store std::string or clearly document and enforce that callers pass only string literals).
-m_function_map.emplace_back(name, f);
+m_function_map.emplace_back(std::string{name}, f);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto register_function_invoker(std::string_view name, Function const& f) -> bool { | |
| if (contains(name)) { | |
| return false; | |
| } | |
| m_function_map.emplace_back(name, f); | |
| return true; | |
| } | |
| auto register_function_invoker(std::string_view name, Function const& f) -> bool { | |
| if (contains(name)) { | |
| return false; | |
| } | |
| m_function_map.emplace_back(std::string{name}, f); | |
| return true; | |
| } |
Collaborator
Author
|
Closed to split into separate PRs. |
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.
Description
This pr replaces
absl::flat_hash_mapwithstd::vectorto fix the crash caused by differences in memory layout across differentabseilversions.This pr also replaces
std::string const&withstd::string_viewand addsnoexceptin functions called during static variable initialization to fix #113.Checklist
breaking change.
Validation performed
g++succeeds.Summary by CodeRabbit