Skip to content

Speed up Signals when there are no receivers.#2229

Merged
asvetlov merged 4 commits intoaio-libs:masterfrom
pfreixes:small_speedup_signals
Aug 28, 2017
Merged

Speed up Signals when there are no receivers.#2229
asvetlov merged 4 commits intoaio-libs:masterfrom
pfreixes:small_speedup_signals

Conversation

@pfreixes
Copy link
Copy Markdown
Contributor

@pfreixes pfreixes commented Aug 27, 2017 •
edited
Loading

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

Pau Freixes added 3 commits August 27, 2017 22:11
Speed up Signals when there are no receivers.
592cfd1 
Reduce the footprint of the `send()` method by a 10% when there are
no receivers connected.
Added change file
46e2fcb
Removed invalid attributes
98f671a
@kxepal
Copy link
Copy Markdown
Member

kxepal commented Aug 27, 2017 •
edited
Loading

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
Copy Markdown
Contributor Author

pfreixes commented Aug 27, 2017

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
Copy Markdown

codecov-io commented Aug 27, 2017 •
edited
Loading

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.

asvetlov
asvetlov requested changes Aug 28, 2017
View reviewed changes
aiohttp/_frozenlist.pyx Outdated
if self.frozen:
raise RuntimeError("Cannot modify frozen list.")

cdef object _fast_len(self):
Copy link
Copy Markdown
Member

@asvetlov asvetlov Aug 28, 2017

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
Copy Markdown
Contributor Author

@pfreixes pfreixes Aug 28, 2017

Choose a reason for hiding this comment

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

done

@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Aug 28, 2017

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
Copy Markdown
Contributor Author

pfreixes commented Aug 28, 2017

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
Copy Markdown
Member

asvetlov commented Aug 28, 2017

Thanks!

@lock
Copy link
Copy Markdown

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.

Reviewers

@asvetlov asvetlov asvetlov approved these changes

Assignees

No one assigned

Labels

bot:chronographer:provided There is a change note present in this PR outdated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pfreixes @kxepal @codecov-io @asvetlov