FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Eliminate the reaper thread by bdraco · Pull Request #349 · python-zeroconf/python-zeroconf · GitHub

/ python-zeroconf Public

Eliminate the reaper thread - #349

Merged
jstasiak merged 1 commit into
python-zeroconf:masterfrom
bdraco:reaper_thread
May 4, 2021
ConversationCommits (1)ChecksFiles changed
Merged

Eliminate the reaper thread#349
jstasiak merged 1 commit into
python-zeroconf:masterfrom
bdraco:reaper_thread

Conversation

bdraco commented May 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

  • Cache is now purged between reads when the interval is reached

  • Reduce locking since we are already making a copy of the readers
    and not reading under the lock

codecov-commenter commented May 3, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #349 (3e2d7bd) into master (523aefb) will increase coverage by 0.32%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
+ Coverage   91.46%   91.78%   +0.32%     
==========================================
  Files           2        2              
  Lines        2682     2678       -4     
  Branches      362      361       -1     
==========================================
+ Hits         2453     2458       +5     
+ Misses        150      147       -3     
+ Partials       79       73       -6
Impacted Files Coverage Δ
zeroconf/__init__.py 89.43% <92.30%> (+0.47%) ⬆️
zeroconf/test.py 94.93% <100.00%> (+0.04%) ⬆️

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 523aefb...3e2d7bd. Read the comment docs.

bdraco force-pushed the reaper_thread branch 2 times, most recently from f27f925 to 12e4812 Compare May 3, 2021 21:16

bdraco commented May 3, 2021

Copy link
Copy Markdown
Member Author

I've pushed this into production testing for a few Home Assistant instances and will circle back after its had a chance to run for a bit.

bdraco commented May 3, 2021

Copy link
Copy Markdown
Member Author

Working well so far. Going to run an object dump for a bit to make sure nothing is leaking

https://www.home-assistant.io/integrations/profiler/#service-profilerstart_log_objects

bdraco commented May 3, 2021

Copy link
Copy Markdown
Member Author

No memory leaks, verified homekit and discovery still working as expected

bdraco marked this pull request as ready for review May 3, 2021 23:32
jstasiak reviewed May 3, 2021
View reviewed changes

jstasiak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks really nice, one question inline

Comment thread zeroconf/__init__.py

bdraco commented May 3, 2021

Copy link
Copy Markdown
Member Author

Ran a py-spy as well to make sure there were no performance regressions

Eliminate the reaper thread
3e2d7bd 
- Cache is now purged between reads when the interval is reached

- Reduce locking since we are already making a copy of the readers
  and not reading under the lock

- Simplify shutdown process
jstasiak merged commit 7816278 into python-zeroconf:master May 4, 2021
bdraco deleted the reaper_thread branch May 4, 2021 08:49
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jstasiak jstasiak approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL