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

bpo-30014: make poll-like selector's modify() method faster #1030

Merged
merged 10 commits into from
Jun 9, 2017
3 changes: 3 additions & 0 deletions Doc/whatsnew/3.7.rst
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,9 @@ Optimizations
expressions <re>`. Searching some patterns can now be up to 20 times faster.
(Contributed by Serhiy Storchaka in :issue:`30285`.)

* :meth:`selectors.EpollSelector.modify`, :meth:`selectors.PollSelector.modify`
and :meth:`selectors.DevpollSelector.modify` may be around 10% faster under
heavy loads. (Contributed by Giampaolo Rodola' in :issue:`30014`)

Build and C API Changes
=======================
Expand Down
30 changes: 29 additions & 1 deletion Lib/selectors.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,6 @@ def unregister(self, fileobj):
return key

def modify(self, fileobj, events, data=None):
# TODO: Subclasses can probably optimize this even further.
try:
key = self._fd_to_key[self._fileobj_lookup(fileobj)]
except KeyError:
Expand Down Expand Up @@ -342,6 +341,8 @@ def select(self, timeout=None):
class _PollLikeSelector(_BaseSelectorImpl):
"""Base class shared between poll, epoll and devpoll selectors."""
_selector_cls = None
_EVENT_READ = None
_EVENT_WRITE = None

def __init__(self):
super().__init__()
Expand Down Expand Up @@ -371,6 +372,33 @@ def unregister(self, fileobj):
pass
return key

def modify(self, fileobj, events, data=None):
try:
key = self._fd_to_key[self._fileobj_lookup(fileobj)]
except KeyError:
raise KeyError(f"{fileobj!r} is not registered") from None

changed = False
if events != key.events:
selector_events = 0
if events & EVENT_READ:
selector_events |= self._EVENT_READ
if events & EVENT_WRITE:
selector_events |= self._EVENT_WRITE
try:
self._selector.modify(key.fd, selector_events)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it isn't time to switch this (and other except clauses in this module) to BaseException. Otherwise a KeyboardInterrupt (in particular) could leave us in a bad state. This has been a recurring problem in asyncio; we eventually are planning to address it in asyncio, and then having it not be a problem here would be a requirement. (Though I'd be okay with filing a separate issue for this instead of folding it into this PR.)

super().unregister(fileobj)
raise
changed = True
if data != key.data:
changed = True

if changed:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How common is the case of unchanged events and data? Wouldn't be faster (in average) always change key and save a time for setting and checking changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how common it is but _replace() is pretty time consuming so I think it's better to avoid it if we can. Checking changed variable is definitively faster than doing this all the time:

            key = key._replace(data=data)
            self._fd_to_key[key.fd] = key

key = key._replace(events=events, data=data)
self._fd_to_key[key.fd] = key
return key

def select(self, timeout=None):
# This is shared between poll() and epoll().
# epoll() has a different signature and handling of timeout parameter.
Expand Down
27 changes: 27 additions & 0 deletions Lib/test/test_selectors.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,33 @@ def test_modify(self):
self.assertFalse(s.register.called)
self.assertFalse(s.unregister.called)

def test_modify_unregister(self):
# Make sure the fd is unregister()ed in case of error on
# modify(): http://bugs.python.org/issue30014
if self.SELECTOR.__name__ == 'EpollSelector':
patch = unittest.mock.patch(
'selectors.EpollSelector._selector_cls')
elif self.SELECTOR.__name__ == 'PollSelector':
patch = unittest.mock.patch(
'selectors.PollSelector._selector_cls')
elif self.SELECTOR.__name__ == 'DevpollSelector':
patch = unittest.mock.patch(
'selectors.DevpollSelector._selector_cls')
else:
raise self.skipTest("")

with patch as m:
m.return_value.modify = unittest.mock.Mock(
side_effect=ZeroDivisionError)
s = self.SELECTOR()
self.addCleanup(s.close)
rd, wr = self.make_socketpair()
s.register(rd, selectors.EVENT_READ)
self.assertEqual(len(s._map), 1)
with self.assertRaises(ZeroDivisionError):
s.modify(rd, selectors.EVENT_WRITE)
self.assertEqual(len(s._map), 0)

def test_close(self):
s = self.SELECTOR()
self.addCleanup(s.close)
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,9 @@ Extension Modules
Library
-------

- bpo-30014: modify() method of poll(), epoll() and devpoll() based classes of
selectors module is around 10% faster. Patch by Giampaolo Rodola'.

- bpo-30418: On Windows, subprocess.Popen.communicate() now also ignore EINVAL
on stdin.write() if the child process is still running but closed the pipe.

Expand Down