fix: Switch from abseil to std::vector for task function and name mapping (fixes #131). - #130
Conversation
WalkthroughThe changes refactor function and function name management by replacing hash map storage with vector-based storage in both Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FunctionManager
participant FunctionNameManager
Client->>FunctionManager: register_function(name, function_ptr)
FunctionManager->>FunctionManager: get(name)
alt name exists
FunctionManager-->>Client: return false
else name not found
FunctionManager->>FunctionManager: emplace_back(name, function_ptr)
FunctionManager-->>Client: return true
end
Client->>FunctionManager: get_function(name)
FunctionManager->>FunctionManager: get(name)
alt name found
FunctionManager-->>Client: return function_ptr
else not found
FunctionManager-->>Client: return nullptr
end
Client->>FunctionNameManager: register_function(name, function_ptr)
FunctionNameManager->>FunctionNameManager: get(function_ptr)
alt pointer exists
FunctionNameManager-->>Client: return false
else pointer not found
FunctionNameManager->>FunctionNameManager: emplace_back(function_ptr, name)
FunctionNameManager-->>Client: return true
end
Client->>FunctionNameManager: get_function_name(function_ptr)
FunctionNameManager->>FunctionNameManager: get(function_ptr)
alt pointer found
FunctionNameManager-->>Client: return name
else not found
FunctionNameManager-->>Client: return nullopt
end
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
♻️ Duplicate comments (1)
src/spider/worker/FunctionManager.hpp (1)
381-386: Same race-condition applies toregister_function_invoker– see previous comment.
🧹 Nitpick comments (2)
src/spider/worker/FunctionManager.hpp (2)
49-50: Potential linear-search hot-spot after replacingflat_hash_mapwithstd::vector
FunctionMapis now a simple vector, so every lookup performed byget_function()or by the tworegister_*helpers is O(n). In typical deployments we might register only a handful of tasks, but if this registry grows (micro-service with hundreds of tasks, plugin system, etc.) start-up time and per-call lookup latency will regress compared with the previous hash look-ups.If keeping a contiguous layout is mandatory (ABI reasons), consider:
- Maintaining the vector sorted by name and using
std::lower_bound(O(log n)), or- Keeping a parallel
std::unordered_map<std::string_view, std::size_t>that stores an index into the vector – still preserves contiguous storage while restoring O(1) average lookup.Either approach keeps the external ABI identical while avoiding the worst-case O(n²) behaviour when all calls go through the registry.
392-394:get(std::string_view)could returnend()sentinel unconditionallyThe new helper returns a
const_iterator; good. Small nit: returningcend()instead ofend()ties the implementation toconstuse only. If future non-const overloads are added you’ll need a second helper. Returningend()keeps it generic without sacrificingconst-correctness.Not blocking, just something to keep in mind.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/spider/core/TaskGraphImpl.hpp(1 hunks)src/spider/worker/FunctionManager.cpp(2 hunks)src/spider/worker/FunctionManager.hpp(3 hunks)src/spider/worker/FunctionNameManager.cpp(2 hunks)src/spider/worker/FunctionNameManager.hpp(3 hunks)tests/worker/test-FunctionManager.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/spider/worker/FunctionNameManager.cpp (2)
src/spider/worker/FunctionManager.cpp (2)
get(110-117)get(110-110)src/spider/worker/FunctionNameManager.hpp (1)
ptr(47-48)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (11)
tests/worker/test-FunctionManager.cpp (2)
59-62: Type update to match interface changeThe code now correctly uses the specialized
spider::core::TaskFunctionPointertype instead of the genericvoid*in thereinterpret_cast. This aligns with the changes in theFunctionNameManagerinterface where function pointers are now stored with a specific type rather than a generic pointer.
66-69: Type consistency improvementThe code consistently uses
spider::core::TaskFunctionPointerfor the positive test case as well, maintaining type safety throughout the test file. This matches the implementation inFunctionNameManagerwhere the storage has changed from a hash map ofvoid*to a vector of typed function pointers.src/spider/core/TaskGraphImpl.hpp (1)
141-141: Type update for consistencyThe cast has been correctly updated from
void const*toTaskFunctionPointer constto match the new signature ofget_function_nameinFunctionNameManager. This change improves type safety by using the specific function pointer type rather than a generic void pointer.src/spider/worker/FunctionNameManager.cpp (3)
5-6: Added required includesThe new includes for
<utility>and<vector>support the changed implementation that uses a vector of pairs instead of a hash map for function name storage.
16-24: New helper method for linear searchThis new
getmethod implements a linear search through the vector of function pointer/name pairs. This approach ensures consistent memory layout across different compiler versions, addressing the core issue withabsl::flat_hash_mapcompatibility.The linear search will have O(n) complexity versus the previous O(1) average case for hash maps, but this tradeoff is acceptable given the compatibility requirements.
26-33: Updated method to use linear searchThe
get_function_namemethod now uses the newgethelper for lookups and accepts the more specificTaskFunctionPointertype. This change maintains the same external behavior while adapting to the new internal storage mechanism.src/spider/worker/FunctionManager.cpp (3)
6-6: Added string_view includeAdded include for
<string_view>to support the more efficient string parameter handling in the newgetmethod.
110-117: Linear search implementationSimilar to the changes in
FunctionNameManager, this newgetmethod implements a linear search through the vector of function name/implementation pairs. The use ofstd::string_viewfor the parameter is a good optimization for read-only string comparisons.This approach ensures consistent memory layout regardless of compiler version, effectively addressing the compatibility issue described in the PR objectives.
119-125: Updated lookup methodThe
get_functionmethod now uses the new linear search approach while maintaining the same external behavior. The return logic is structured well, providing a pointer to the found function or nullptr if not found.src/spider/worker/FunctionManager.hpp (1)
365-377:register_functionis not thread-safe – guard the vector or document single-threaded usage
m_function_map.emplace_back(...)mutates a global singleton without any synchronisation. If two shared libraries (or two different static initialisation units) attempt to register tasks concurrently, a data race will occur and the behaviour is undefined.Please either:
+#include <mutex> + class FunctionManager { ... private: + mutable std::mutex m_mutex;and lock it inside
register_function/register_function_invokerandget, or explicitly document that all registrations must happen during single-threaded static initialisation before any worker threads are spawned.src/spider/worker/FunctionNameManager.hpp (1)
39-45: Concurrent registrations can race – add a mutex or clarify lifecycleAs with
FunctionManager,m_name_map.emplace_back(...)performs an unsynchronised write on a global singleton. Protect the vector with a mutex or clearly state (in docs / comments) that all task name registrations must be completed before multi-threaded execution begins.
@coderabbitai What race-condition are you referring to in this comment:
Can you link to the original comment? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@davidlion I apologize for the confusion. I've checked the code and found that the race condition comment refers to a concurrent access issue in both The race condition exists in the "check-then-act" pattern used in both methods. Here's what happens in
This same pattern appears in Without proper synchronization (like a mutex), if two threads call these methods concurrently with the same function name:
This race condition wasn't introduced by this PR, but changing from a hash map to a vector doesn't address it. In a concurrent environment, this could lead to duplicate entries or undefined behavior when looking up functions by name. Would you like me to suggest a solution for addressing this race condition? 🏁 Script executed: Length of output: 14391 🏁 Script executed: Length of output: 792 🏁 Script executed: Length of output: 4502 @davidlion I found the original comment. The race condition refers to a thread safety issue in From the PR review comments:
Both if (m_function_map.cend() != get(name)) {
return false;
}
m_function_map.emplace_back(name, ...);The race condition occurs because
Since there's no synchronization mechanism (like a mutex), this creates a data race with undefined behavior. ✏️ Learnings added
|
sitaowang1998
changed the title
std::vector for function and name mapping.std::vector for task function and name mapping.
davidlion
changed the title
std::vector for task function and name mapping.abseil to std::vector for task function and name mapping (fixes #131).
2 participants
Description
FunctionMapandFunctionNameMapusesabsl::flat_hash_mapto store the mapping between functions and names. However, difference inabseilversions between a task executor and a task library means that the task executor'sflat_hash_maphas different memory layout than the map stored by the task library, and the reading of the map fails and crashes.This pr solves this issue by using a plain
std::vectorfor the map. The data are stored as a consecutive array, and layout remains the same across different compiler versions. Fixes #131.Checklist
breaking change.
Validation performed
Spidercomponents compiled with gcc-13.Summary by CodeRabbit