Skip to content

gh-106554: Replace _BaseSelectorImpl._key_from_fd with a simple get #106555

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

Merged
merged 7 commits into from
Jul 14, 2023

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Jul 9, 2023

`_key_from_fd` re-implemented `.get()` and can be removed
@bdraco
Copy link
Contributor Author

bdraco commented Jul 12, 2023

import timeit
import math
import select
import os
from selectors import EpollSelector, EVENT_WRITE, EVENT_READ


class OriginalEpollSelector(EpollSelector):
    def select(self, timeout=None):
        if timeout is None:
            timeout = -1
        elif timeout <= 0:
            timeout = 0
        else:
            # epoll_wait() has a resolution of 1 millisecond, round away
            # from zero to wait *at least* timeout seconds.
            timeout = math.ceil(timeout * 1e3) * 1e-3
        # epoll_wait() expects `maxevents` to be greater than zero;
        # we want to make sure that `select()` can be called when no
        # FD is registered.
        max_ev = max(len(self._fd_to_key), 1)
        ready = []
        try:
            fd_event_list = self._selector.poll(timeout, max_ev)
        except InterruptedError:
            return ready
        for fd, event in fd_event_list:
            events = 0
            if event & ~select.EPOLLIN:
                events |= EVENT_WRITE
            if event & ~select.EPOLLOUT:
                events |= EVENT_READ

            key = self._key_from_fd(fd)
            if key:
                ready.append((key, events & key.events))
        return ready


class NewEpollSelector(EpollSelector):
    def select(self, timeout=None):
        if timeout is None:
            timeout = -1
        elif timeout <= 0:
            timeout = 0
        else:
            # epoll_wait() has a resolution of 1 millisecond, round away
            # from zero to wait *at least* timeout seconds.
            timeout = math.ceil(timeout * 1e3) * 1e-3
        # epoll_wait() expects `maxevents` to be greater than zero;
        # we want to make sure that `select()` can be called when no
        # FD is registered.
        max_ev = max(len(self._fd_to_key), 1)
        ready = []
        try:
            fd_event_list = self._selector.poll(timeout, max_ev)
        except InterruptedError:
            return ready
        for fd, event in fd_event_list:
            events = 0
            if event & ~select.EPOLLIN:
                events |= EVENT_WRITE
            if event & ~select.EPOLLOUT:
                events |= EVENT_READ

            key = self._fd_to_key.get(fd)
            if key:
                ready.append((key, events & key.events))
        return ready


original_epoll = OriginalEpollSelector()
new_epoll = NewEpollSelector()


for _ in range(512):
    r, w = os.pipe()
    os.write(w, b"a")
    original_epoll.register(r, EVENT_READ)
    new_epoll.register(r, EVENT_READ)


original_time = timeit.timeit(
    "selector.select()",
    number=100000,
    globals={"selector": original_epoll},
)
new_time = timeit.timeit(
    "selector.select()",
    number=100000,
    globals={"selector": new_epoll},
)

print("original: %s" % original_time)
print("new: %s" % new_time)

@bdraco
Copy link
Contributor Author

bdraco commented Jul 12, 2023

512 pipes

original: 9.91608710300352
new: 9.240273159986828

1024 pipes

original: 20.881464588004746
new: 19.727144259988563

@bdraco
Copy link
Contributor Author

bdraco commented Jul 12, 2023

A more aggressive change to reduce the repeated calls inside the loop could shave some more time off

512 pipes

original: 10.050354178994894
new: 8.222623112000292

1024 pipes

original: 20.87793983600568
new: 17.375853078992805

something like the below (but outside the scope here)

NOT_EPOLLIN = ~select.EPOLLIN
NOT_EPOLLOUT = ~select.EPOLLOUT

class NewEpollSelector(EpollSelector):
    def select(self, timeout=None):
        if timeout is None:
            timeout = -1
        elif timeout <= 0:
            timeout = 0
        else:
            # epoll_wait() has a resolution of 1 millisecond, round away
            # from zero to wait *at least* timeout seconds.
            timeout = math.ceil(timeout * 1e3) * 1e-3
        # epoll_wait() expects `maxevents` to be greater than zero;
        # we want to make sure that `select()` can be called when no
        # FD is registered.
        max_ev = max(len(self._fd_to_key), 1)
        ready = []
        try:
            fd_event_list = self._selector.poll(timeout, max_ev)
        except InterruptedError:
            return ready

        for fd, event in fd_event_list:
            key = self._fd_to_key.get(fd)
            if key:
                ready.append(
                    (
                        key,
                        (
                            (event & NOT_EPOLLIN and EVENT_WRITE)
                            | (event & NOT_EPOLLOUT and EVENT_READ)
                        )
                        & key.events,
                    )
                )
        return ready

Getting rid of the max and writing it as len(self._fd_to_key) or 1 would probably help as well

Screenshot 2023-07-13 at 3 32 14 PM
NOT_EPOLLIN = ~select.EPOLLIN
NOT_EPOLLOUT = ~select.EPOLLOUT

class NewEpollSelector(EpollSelector):
    def select(self, timeout=None):
        if timeout is None:
            timeout = -1
        elif timeout <= 0:
            timeout = 0
        else:
            # epoll_wait() has a resolution of 1 millisecond, round away
            # from zero to wait *at least* timeout seconds.
            timeout = math.ceil(timeout * 1e3) * 1e-3
        # epoll_wait() expects `maxevents` to be greater than zero;
        # we want to make sure that `select()` can be called when no
        # FD is registered.
        max_ev = len(self._fd_to_key) or 1
        ready = []
        try:
            fd_event_list = self._selector.poll(timeout, max_ev)
        except InterruptedError:
            return ready

        for fd, event in fd_event_list:
            key = self._fd_to_key.get(fd)
            if key:
                ready.append(
                    (
                        key,
                        (
                            (event & NOT_EPOLLIN and EVENT_WRITE)
                            | (event & NOT_EPOLLOUT and EVENT_READ)
                        )
                        & key.events,
                    )
                )
        return ready

at 512 (on a different system)

original: 11.723339454038069
new: 9.671272879000753

@bdraco
Copy link
Contributor Author

bdraco commented Jul 12, 2023

KqueueSelector could shave off some more time as well with something like

        def select(self, timeout=None):
            timeout = None if timeout is None else max(timeout, 0)
            # If max_ev is 0, kqueue will ignore the timeout. For consistent
            # behavior with the other selector classes, we prevent that here
            # (using max). See https://bugs.python.org/issue29255
            max_ev = max(len(self._fd_to_key), 1)
            ready = []
            try:
                kev_list = self._selector.control(None, max_ev, timeout)
            except InterruptedError:
                return ready
            for kev in kev_list:
                key = self._fd_to_key.get(kev.ident)
                if key:
                    flag = kev.filter
                    if flag == select.KQ_FILTER_READ:
                        events = EVENT_READ
                    elif flag == select.KQ_FILTER_WRITE:
                        events = EVENT_WRITE
                    else:
                        events = 0
                    ready.append((key, events & key.events))
            return ready

@bdraco
Copy link
Contributor Author

bdraco commented Jul 13, 2023

With #106555 (comment) the selector overhead falls below the cost of socket.recv in the profiles. Its still high on the list at #4 for my home assistant install, eclipsed only by socket.recv, _run_once and some bluetooth overhead.

@methane methane merged commit aeef859 into python:main Jul 14, 2023
@bdraco
Copy link
Contributor Author

bdraco commented Jul 14, 2023

Thanks. Will work on the follow ups from above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants