TP Reactor was excessively calling select #1157
Conversation
The TP Reactor implementation of the clear_dispatch_mask was implemented by clearing the entire ready set instead of just removing the handler that was suspended. This forced the TP Reactor to potentially dispatch only a single handler before re-calling an expensive select() call and restarting the dispatch with the new set returned from select(). This heavily favored lower order fds and was inefficient. This originated with the fix for ACE bugzilla 1890 which appears to have mainly focused on the single threaded select reactor.
|
What about removing the implementation in TP_Reactor completely and just use the implementation in ACE_Select_Reactor_Impl? |
|
Sure thing, can do. |
|
Though the base class sets state_changed_ to true, won't that reset all the handles in get_event_for_dispatching? I can try to take a deeper look comparing with the older behavior as this was degrading performance for us. |
WalkthroughThis pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TP_Reactor
Caller->>TP_Reactor: clear_dispatch_mask(handle, mask)
TP_Reactor->>TP_Reactor: Check if mask has READ_MASK or ACCEPT_MASK
TP_Reactor->>TP_Reactor: Clear read mask if condition met
TP_Reactor->>TP_Reactor: Check if mask has WRITE_MASK
TP_Reactor->>TP_Reactor: Clear write mask if condition met
TP_Reactor->>TP_Reactor: Check if mask has EXCEPT_MASK
TP_Reactor->>TP_Reactor: Clear exception mask if condition met
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ACE/ace/TP_Reactor.inl (1)
107-109: Consider adding documentation.Since this is a significant behavioral change, consider adding a brief comment explaining the purpose of selective mask clearing and its impact on performance.
Add this documentation above the method:
+/** + * Selectively clear dispatch masks for a handle based on the provided mask. + * This optimizes reactor performance by preventing unnecessary select() calls + * and ensures fair handler dispatching. + */ ACE_INLINE void ACE_TP_Reactor::clear_dispatch_mask (ACE_HANDLE handle, ACE_Reactor_Mask mask)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/ace/TP_Reactor.inl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
ACE/ace/TP_Reactor.inl (1)
107-123: LGTM! The implementation aligns with PR objectives.The selective mask clearing approach correctly addresses the performance issue by preventing unnecessary
select()calls. This should improve the fairness of handler dispatching.Let's verify that all callers have been updated to use the new method signature:
✅ Verification successful
Review Verification: All Caller Updates Confirmed
Based on the executed script output, every caller of
clear_dispatch_maskacross the codebase passes two parameters (the handle and the mask), matching the updated method signature. This confirms that the changes inTP_Reactor.inlhave been correctly propagated to all usage sites.
- All instances in
TP_Reactor.inl,TP_Reactor.h,Select_Reactor_Base.{h,cpp},Select_Reactor_T.cpp, andPriority_Reactor.cppcall the function with both required parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all callers of clear_dispatch_mask to ensure they've been updated # Search for method calls rg -A 2 "clear_dispatch_mask" --type cppLength of output: 1794
The TP Reactor implementation of the clear_dispatch_mask was implemented
by clearing the entire ready set instead of just removing the handler
that was suspended. This forced the TP Reactor to potentially dispatch
only a single handler before re-calling an expensive select() call and
restarting the dispatch with the new set returned from select(). This
heavily favored lower order fds and was inefficient. This originated
with the fix for ACE bugzilla 1890 which appears to have mainly focused
on the single threaded select reactor.
Summary by CodeRabbit