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

Speed up Signals when there are no receivers. #2229

Merged
merged 4 commits into from
Aug 28, 2017

Conversation

pfreixes
Copy link
Contributor

@pfreixes pfreixes commented Aug 27, 2017

What do these changes do?

Reduce the footprint of the send() method by a 10% when there are
no receivers connected.

Before

$ python ../test_aiohttp_signals.py
Signals per second: 2296186.5549501474

After

$ python ../test_aiohttp_signals.py
Signals per second: 2759791.7854124783

Script used [1]

[1] https://gist.github.com/pfreixes/baea4ecc7b5380d534d60a8c1e8da32f

Are there changes in behavior for the user?

No

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Add a new news fragment into the changes folder
    • name it <issue_id>.<type> for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Reduce the footprint of the `send()` method by a 10% when there are
no receivers connected.
@kxepal
Copy link
Member

kxepal commented Aug 27, 2017

Nice! Since this is a FrozenList, his length is constant. What if you compute length once at __init__ and return that static value from __len__ ?

Update: I always forgot that it's not frozen like frozenset. Ok, what if do the same - compute length once on freeze while keep it dynamic while it's not frozen?

@pfreixes
Copy link
Contributor Author

From what I've seen the freeze is done explicitly by the developer at some moment, would be needed calculate it at the moment of the freeze() call. But, seeing the Signal implementation seems that this characteristic is not really used to determine if a Signal can be used or not, looks like that the freeze only avoids modify the items saved from that moment. Why ? no fucking idea :(

@codecov-io
Copy link

codecov-io commented Aug 27, 2017

Codecov Report

Merging #2229 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2229   +/-   ##
=======================================
  Coverage   97.15%   97.15%           
=======================================
  Files          39       39           
  Lines        7899     7899           
  Branches     1369     1369           
=======================================
  Hits         7674     7674           
  Misses        100      100           
  Partials      125      125

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7658d4...cc3eb8d. Read the comment docs.

@@ -18,6 +18,9 @@ cdef class FrozenList:
if self.frozen:
raise RuntimeError("Cannot modify frozen list.")

cdef object _fast_len(self):
Copy link
Member

Choose a reason for hiding this comment

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

The function is used only once, please inline it -- the speed should be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@asvetlov
Copy link
Member

You've raised two separate questions:

  1. Why raising non-frozen signals are allowed?

  2. Does length calculation on .freeze() makes sense?

  3. It's implementation detail. I just forgot to add check for .frozen in Signal.send().
    I see no harm in behavior changing, we could start from raising a warning.

  4. Technically we could add cdef int _len attribute but I pretty sure PyList_Size() is pretty fast too. Extra field adds unnecessary complexity.

@pfreixes
Copy link
Contributor Author

Yes, the cost of PyList_Size() vs doing explicitly with an attribute is negligible. And makes the code more complicated.

@asvetlov asvetlov merged commit d2236cf into aio-libs:master Aug 28, 2017
@asvetlov
Copy link
Member

Thanks!

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants