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

giampaolo · 2017-04-07T12:48:42Z

http://bugs.python.org/issue30014
This applies to poll(), epoll() and devpoll() selectors, and makes modify() method up to 11.4% faster under heavy loads.

…selector's modify() method instead of un/register()ing the fd

mention-bot · 2017-04-07T12:48:42Z

@giampaolo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cf-natali and @1st1 to be potential reviewers.

vstinner

The overall change LGTM, but I have some remarks, and I would prefer to see Charles-François Natali approve the change first, since he is the maintainer of the module.

vstinner · 2017-04-07T12:52:45Z

Doc/whatsnew/3.7.rst

@@ -169,6 +169,8 @@ Optimizations
  using the :func:`os.scandir` function.
  (Contributed by Serhiy Storchaka in :issue:`25996`.)

+* :meth:`selectors.DefaultSelector.modify` on Linux, OSX and Solaris is around


"DefaultSelector" depends on the platform, I would prefer to mention concrete classes: PollSelector, EpollSelector and DevpollSelector.

vstinner · 2017-04-07T12:53:08Z

Lib/selectors.py

+            try:
+                key = self._fd_to_key[self._fileobj_lookup(fileobj)]
+            except KeyError:
+                raise KeyError("{!r} is not registered".format(fileobj)) from None


nitpick: please add an empty line for readability.

vstinner · 2017-04-07T12:55:26Z

Lib/selectors.py

+                try:
+                    self._epoll.modify(key.fd, selector_events)
+                except BaseException:
+                    super().unregister(fileobj)


The doc says that modify() works as unregister+register, so basically the doc is still ok after your change.

Question: is it worth it to mention that modify() unregisters the FD on error, and maybe even write an unit test for that?

Makes sense. I added a test case for that.

berkerpeksag · 2017-04-19T12:58:27Z

Doc/whatsnew/3.7.rst

@@ -169,6 +169,9 @@ Optimizations
  using the :func:`os.scandir` function.
  (Contributed by Serhiy Storchaka in :issue:`25996`.)

+* :meth:`selectors.EpollSelector.modify`, :meth:`selectors.PollSelector.modify`
+  and :meth:`selectors.DevpollSelector.modify` are around 1.5x faster.
+  (Contributed by Giampaolo Rodola')


(Contributed by Giampaolo Rodola' in :issue:`30014`.)

berkerpeksag · 2017-04-19T13:00:06Z

Lib/selectors.py

+            try:
+                key = self._fd_to_key[self._fileobj_lookup(fileobj)]
+            except KeyError:
+                raise KeyError("{!r} is not registered".format(fileobj)) from None


f"{fileobj!r} is not registered" would make the shorter and cleaner.

berkerpeksag · 2017-04-19T13:05:05Z

Lib/test/test_selectors.py

+        elif self.SELECTOR.__name__ == 'DevpollSelector':
+            patch = unittest.mock.patch('selectors.select.devpoll')
+        else:
+            return unittest.skip("")


unittest.skip() is a decorator. Using raise self.skipTest() or raise unittest.SkipTest() would be more appropriate for this use case.

berkerpeksag · 2017-04-19T13:10:56Z

Misc/NEWS

@@ -303,6 +303,10 @@ Extension Modules
 Library
 -------

+- bpo-30014: selectors.EpollSelector.modify, selectors.PollSelector.modify
+  and selectors.DevpollSelector.modify are around 1.5x faster.
+  (Contributed by Giampaolo Rodola')


Contributed by -> Patch by (but you may skip this and only attribute your name in the whatsnew document since you're already a core developer :))

… call

giampaolo · 2017-06-07T14:09:19Z

OK, I addressed all comments in the PR.

giampaolo · 2017-06-07T14:10:11Z

@cf-natali

serhiy-storchaka · 2017-06-07T14:22:49Z

Lib/selectors.py

+        if data != key.data:
+            changed = True
+
+        if changed:


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?

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

serhiy-storchaka · 2017-06-07T14:31:52Z

Lib/selectors.py

+                selector_events |= self._EVENT_WRITE
+            try:
+                self._selector.modify(key.fd, selector_events)
+            except BaseException:


Why BaseException? _PollLikeSelector.register() uses Exception.

If BaseException is used intentionally, it can be omitted, the bare except has the same effect.

Yes, indeed this should be Exception

serhiy-storchaka · 2017-06-07T14:37:47Z

Doc/whatsnew/3.7.rst

@@ -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 up to 11.4% faster under


Such precision is unbelievable.

This number comes from the benchmark script provided in http://bugs.python.org/issue30581. Perhaps it's better to rephrase this as "around 10% faster"?

This would look better to me.

serhiy-storchaka

Technically the patch looks correct to me (but needs a Misc/NEWS entry). But I don't know whether is is worth to add additional coupling for 10% speedup. If the subclass of _PollLikeSelector implements register() or unregister(), it would need to reimplement also modify().

serhiy-storchaka · 2017-06-07T15:04:28Z

Doc/whatsnew/3.7.rst

@@ -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 up to 11.4% faster under


This would look better to me.

All of my comments have been addressed.

giampaolo · 2017-06-09T12:18:58Z

I think this is good to go. Any objections?

vstinner · 2017-06-09T12:49:41Z

I think this is good to go. Any objections?

I asked why not replacing expensive "if data != key.data:" with "if data is not key.data:", but I didn't get an answer. I suggest to use cheap "is not" for speed but also avoid weird bugs on strange comparison functions (like math.nan != math.nan? :-p).

serhiy-storchaka · 2017-06-09T13:00:01Z

I have no objections.

vstinner

I would like to see my question answered ;-)

vstinner · 2017-06-07T14:00:13Z

Lib/selectors.py

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


Comparison might be expensive. Would it ok to use "is not"?

It's ok to use "!=" if "is not" wouldn't work.

giampaolo · 2017-06-09T15:24:04Z

I asked why not replacing expensive "if data != key.data:" with "if data is not key.data:", but I didn't get an answer.

Oh sorry, I missed your comment. Mmm... why would that be faster? I think I'm missing something.
FWIW also _BaseSelectorImpl.modify() method uses != instead of is not.

vstinner · 2017-06-09T16:29:46Z

Oh sorry, I missed your comment. Mmm... why would that be faster? I think I'm missing something.

If data is a 1 GB string, data1 != data2 requires to compare all bytes. If the object implements custom eq() method, I don't think that it's a good idea to call it here.

IHMO we should only optimize the obvious case: when the same data object is passed again (but modify is called to modify tracked events for example).

FWIW also _BaseSelectorImpl.modify() method uses != instead of is not.

I suggest to also modify the base method in that case.

giampaolo · 2017-06-09T16:43:24Z

is not is indeed a bit faster:

~/svn/cpython {bpo-30014-selectors-modify-speedup|MERGING}$ ./python -m timeit "a = '1'*1024; b = '1' * 1024 + '2'" "a != b"
1000000 loops, best of 5: 315 nsec per loop
~/svn/cpython {bpo-30014-selectors-modify-speedup|MERGING}$ ./python -m timeit "a = '1'*1024; b = '1' * 1024 + '2'" "a is not b"
1000000 loops, best of 5: 297 nsec per loop

OK, I added this change.

gvanrossum · 2017-06-09T17:27:10Z

@serhiy-storchaka I no longer know this code very well -- do you really want my review in addition to all the other reviews you've already had? Is there a specific question you have for me?

serhiy-storchaka · 2017-06-09T17:40:54Z

@gvanrossum, nothing specific. Just you were one of the early committers to this module (with Charles-François Natali which seems not available now) and once had committed a patch related to optimization of the modify() method (d0786a1). I though that you can know something missed by other reviewers.

gvanrossum · 2017-06-09T18:17:49Z

Lib/selectors.py

        try:
            key = self._fd_to_key[self._fileobj_lookup(fileobj)]
        except KeyError:
            raise KeyError("{!r} is not registered".format(fileobj)) from None
        if events != key.events:
            self.unregister(fileobj)
            key = self.register(fileobj, events, data)
-        elif data != key.data:
+        elif data is not key.data:


I disagree with this change. I've found nothing in the docs constraining the __eq__ sematics of data. In the common cases, where data is either None or unchanged, __eq__ typically still takes a shortcut when comparing the object to itself. So the story about comparing two 1Gb strings seems unlikely -- either it's the same string, in which case it takes a shortcut, or it's two different strings, in which case the comparison bails at the first difference. In case the data is e.g. a list, and the new data is a different list containing the same values, this "optimization" would actually be a slow-down. In general it smells superstitious. I think the code should show the intended semantics.

Thanks for chiming in. I changed this back.

gvanrossum · 2017-06-09T18:20:34Z

Lib/selectors.py

+                selector_events |= self._EVENT_WRITE
+            try:
+                self._selector.modify(key.fd, selector_events)
+            except Exception:


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.)

gvanrossum · 2017-06-09T18:21:07Z

Lib/selectors.py

+                super().unregister(fileobj)
+                raise
+            changed = True
+        if data is not key.data:


Again, I think != is not generally slower and closer to the promised semantics.

gvanrossum · 2017-06-09T20:14:10Z

I'm happy now. The BaseException thing could happen another time.

vstinner · 2017-06-09T20:23:28Z

It's always possible to find something to enhance, but IMHO the change is now good enough and so I merged it.

Ok for != vs is not, Guido opinion matters more than mine since he is the coauthor with Charles-François Natali if I recall correctly.

vstinner · 2017-06-09T20:25:39Z

Thanks Giampaolo for this nice opyimization! I tried to convince one or two times a few months ago, but you was motivated than me and succeeded to implement a nice change to factorize the code ;-)

vstinner · 2017-06-09T20:27:56Z

I'm happy now. The BaseException thing could happen another time.

Yeah please, it deserves its own change. When an except block ends with "raise", I prefer "except:" or "except BaseException:".

giampaolo · 2017-06-10T18:16:03Z

I'm happy now. The BaseException thing could happen another time.

Yeah please, it deserves its own change. When an except block ends with "raise", I prefer "except:" or "except BaseException:".

http://bugs.python.org/issue30624
#2082

#30014: make selectors.DefaultSelector.modify() faster by relying on …

11f5491

…selector's modify() method instead of un/register()ing the fd

the-knights-who-say-ni added the CLA signed label Apr 7, 2017

vstinner changed the title ~~#30014: make selectors.DefaultSelector.modify() faster~~ bpo-30014: make selectors.DefaultSelector.modify() faster Apr 7, 2017

vstinner reviewed Apr 7, 2017

View reviewed changes

#30014: add unit test

43d5df1

berkerpeksag previously requested changes Apr 19, 2017

View reviewed changes

giampaolo added 5 commits May 20, 2017 12:23

merge from master

614ef4a

Merge branch 'master' into bpo-30014-selectors-modify-speedup

0a4720c

speedup poll/epoll/devpoll modify() method by using internal modify()…

00654cd

… call

update doc

951e5f7

address PR comments

e153f84

giampaolo changed the title ~~bpo-30014: make selectors.DefaultSelector.modify() faster~~ bpo-30014: make poll-like selector's modify() method faster Jun 7, 2017

serhiy-storchaka reviewed Jun 7, 2017

View reviewed changes

update NEWS entries

d9a21b9

serhiy-storchaka requested a review from gvanrossum June 9, 2017 12:43

vstinner requested changes Jun 9, 2017

View reviewed changes

compare data by using 'is not' instead of '!=' (faster)

70fae77

gvanrossum requested changes Jun 9, 2017

View reviewed changes

use != instead of 'is not'

964f175

gvanrossum approved these changes Jun 9, 2017

View reviewed changes

vstinner merged commit fbfaa6f into master Jun 9, 2017

vstinner deleted the bpo-30014-selectors-modify-speedup branch June 9, 2017 20:23

gvanrossum mentioned this pull request Jun 12, 2017

bpo-30624 / selectors: use bare except clause #2082

Merged

brianmaissy mentioned this pull request May 10, 2021

FileNotFoundError when connecting to postgres if fd is closed and then reopened aio-libs/aiopg#837

Open

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

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

Conversation

giampaolo commented Apr 7, 2017 • edited Loading

mention-bot commented Apr 7, 2017

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo commented Jun 7, 2017

giampaolo commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo commented Jun 9, 2017

vstinner commented Jun 9, 2017

serhiy-storchaka commented Jun 9, 2017

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo commented Jun 9, 2017

vstinner commented Jun 9, 2017

giampaolo commented Jun 9, 2017

gvanrossum commented Jun 9, 2017

serhiy-storchaka commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Jun 9, 2017

vstinner commented Jun 9, 2017

vstinner commented Jun 9, 2017

vstinner commented Jun 9, 2017

giampaolo commented Jun 10, 2017

giampaolo commented Apr 7, 2017 •

edited

Loading

giampaolo Jun 7, 2017 •

edited

Loading