Reduce bloat due to statics#519
Conversation
Avoid disseminating every compile unit including Message.h with TransportNames and TransportTypes and the associated unordered_map helper methods (e.g. murmur_hash).
📝 WalkthroughWalkthroughThe pull request introduces a significant refactoring of transport-related definitions in the FairMQ library. A new header file Changes
Sequence DiagramsequenceDiagram
participant Channel
participant TransportEnum
Channel->>TransportEnum: Get Transport Type
TransportEnum-->>Channel: Return Transport Type
Channel->>TransportEnum: Update Transport
TransportEnum->>Channel: Validate Transport
Channel->>Channel: Invalidate Current Configuration
The sequence diagram illustrates the interaction between the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fairmq/Channel.cxx (1)
387-392: Document invalidation side effect in UpdateTransport.The implementation is correct, but it's important to document that calling this method invalidates the channel, requiring revalidation before use.
Add a comment in the header file's method declaration:
/// Set channel transport /// @param transport transport string ("default", "zeromq" or "shmem") + /// @note Calling this method invalidates the channel, requiring revalidation before use. void UpdateTransport(const std::string& transport);fairmq/CMakeLists.txt (1)
66-66: LGTM! Consider adding documentation.The addition of
TransportEnum.htoFAIRMQ_PUBLIC_HEADER_FILESis correct and maintains proper header organization. Consider adding a comment in the CMakeLists.txt to document the purpose of this header and its relationship to transport management.TransportFactory.h Transports.h + # Contains core transport enumerations used across the codebase TransportEnum.h UnmanagedRegion.h
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
fairmq/CMakeLists.txt(1 hunks)fairmq/Channel.cxx(2 hunks)fairmq/Channel.h(3 hunks)fairmq/Device.cxx(1 hunks)fairmq/Device.h(1 hunks)fairmq/Message.h(1 hunks)fairmq/TransportEnum.h(1 hunks)fairmq/TransportFactory.h(1 hunks)fairmq/Transports.h(1 hunks)fairmq/UnmanagedRegion.h(1 hunks)fairmq/shmem/Message.h(1 hunks)fairmq/shmem/Segment.h(1 hunks)fairmq/shmem/UnmanagedRegion.h(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- fairmq/TransportEnum.h
- fairmq/UnmanagedRegion.h
- fairmq/Device.cxx
- fairmq/Device.h
- fairmq/shmem/Segment.h
- fairmq/TransportFactory.h
- fairmq/shmem/Message.h
- fairmq/shmem/UnmanagedRegion.h
🔇 Additional comments (5)
fairmq/Transports.h (1)
13-13: LGTM! Changes align with PR objectives.The relocation of the
Transportenum to a separate header while keeping the static maps in this file effectively reduces unnecessary inclusions across compile units.fairmq/Message.h (1)
13-13: LGTM! Minimizes header dependencies.The change to include only
TransportEnum.hinstead of the fullTransports.hreduces compile-time dependencies, as this file only needs the enum definition.fairmq/Channel.cxx (1)
15-15: LGTM! Appropriate location for including Transports.h.Including
Transports.hhere is correct as this is where the static maps are needed for the method implementations.fairmq/Channel.h (1)
17-17: LGTM! Good separation of interface and implementation.The changes effectively:
- Minimize header dependencies by including only
TransportEnum.h- Reduce header bloat by moving implementations to the source file
Also applies to: 148-152, 224-224
fairmq/CMakeLists.txt (1)
66-66: Verify if both transport headers are neededWhile adding
TransportEnum.haligns with reducing static bloat, I notice that bothTransports.handTransportEnum.hare listed inFAIRMQ_PUBLIC_HEADER_FILES. Consider:
- Is
Transports.hstill needed as a public header?- Should we deprecate
Transports.hin favor ofTransportEnum.h?Let's check the usage and dependencies:
✅ Verification successful
Both transport headers serve distinct purposes and should be kept
The split between
TransportEnum.h(enum definition) andTransports.h(string mappings and utilities) is well-designed and aligns with the goal of reducing bloat. Files that only need the enum can now include the minimalTransportEnum.h, while those needing the conversion utilities can includeTransports.h.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Transports.h is still being used rg -l "Transports\.h" --type cpp # Check what's included in both headers echo "=== Transports.h contents ===" cat fairmq/Transports.h || echo "File not found" echo -e "\n=== TransportEnum.h contents ===" cat fairmq/TransportEnum.h || echo "File not found"Length of output: 3029
|
Ehm... Pardon my GenX-itude... Am I supposed to dialog / address the issues reported by coderabbit? In particular the nitpicks seems to be rather useless... |
|
Feel free to ignore it if it's not useful. |
dennisklein
left a comment
There was a problem hiding this comment.
Looks fine to me. Thx and a happy new year :)
Avoid disseminating every compile unit including Message.h with TransportNames and
TransportTypes and the associated unordered_map helper methods (e.g.
murmur_hash).