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

Add "GC" for dispatcher workers #657

Merged
merged 5 commits into from Jun 26, 2022
Merged

Add "GC" for dispatcher workers #657

merged 5 commits into from Jun 26, 2022

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Jun 17, 2022

We were leaking workers, this PR tries to fix this.


Solution that we ended up using:

  • Dispatcher tracks the current & maximum number of active workers
  • If there are more workers than the maximum number of active workers, Dispatcher removes all inactive workers

@Hirrolot Hirrolot self-requested a review June 26, 2022 17:34
src/dispatching/dispatcher.rs Outdated Show resolved Hide resolved
@WaffleLapkin
Copy link
Member Author

@Hirrolot I've addressed your concern about "gc_worker_count_trigger" parameter. Now teloxide automatically sets this parameter to the maximum number of active workers at the same time.

Pros: you don't need to choose this parameter, it's automatically set to a reasonable value.
Cons: there is no way to decrease this parameter, if normally bot has 1 user, but at peek it has 1000 users, then we are still wasting memory.

IMO this is good-enough as it is and we can improve this algorithm later.

src/dispatching/dispatcher.rs Outdated Show resolved Hide resolved
@WaffleLapkin WaffleLapkin merged commit 19eca5d into dev Jun 26, 2022
@WaffleLapkin WaffleLapkin deleted the gc_for_workers branch June 26, 2022 22:41
WaffleLapkin added a commit that referenced this pull request Nov 1, 2022
Add "GC" for dispatcher workers

Former-commit-id: 19eca5d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants