Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TP Reactor was excessively calling select #1157

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danielkempenich
Copy link

@danielkempenich danielkempenich commented Jul 14, 2020

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.

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.
@jwillemsen jwillemsen added the needs review Needs to be reviewed label Jul 17, 2020
@jwillemsen
Copy link
Member

What about removing the implementation in TP_Reactor completely and just use the implementation in ACE_Select_Reactor_Impl?

@danielkempenich
Copy link
Author

Sure thing, can do.

@danielkempenich
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Needs to be reviewed
Development

Successfully merging this pull request may close these issues.

2 participants