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

Only send Tick messages to live threads #1639

Merged
merged 7 commits into from
Sep 22, 2020

Conversation

eddyashton
Copy link
Member

Fixes a memory growth issue, where messages would be queued indefinitely for inactive threads.

The real fix is to loop < thread_count rather than : tasks. The rest of the diff is from pulling the inline lambda into tick_cb.

@eddyashton eddyashton requested a review from a team as a code owner September 22, 2020 14:33
@ghost
Copy link

ghost commented Sep 22, 2020

tick_only_live_threads@13050 aka 20200922.42 vs master ewma over 50 builds from 12438 to 13041
images

@achamayou
Copy link
Member

@eddyashton I suggest we inaugurate adding to the changelog on this PR.

@eddyashton
Copy link
Member Author

@eddyashton I suggest we inaugurate adding to the changelog on this PR.

Done. There's a slightly awkward chicken-and-egg issue here - we want to refer to PR numbers in the changelog, but the numbers don't exist until the PR is created. So if we know we're going to put the current change into the Changelog, we need at least one post-PR-creation update to the branch. Minor, but worth noting.

@eddyashton eddyashton merged commit 4b4479b into microsoft:master Sep 22, 2020
@achamayou
Copy link
Member

@eddyashton this is less of a problem if we point to issues or documentation, but a good point nonetheless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants