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

Switched the BlockEvent stream to Tokio broadcast #1889

Merged
merged 1 commit into from
May 20, 2020

Conversation

neonknight64
Copy link
Contributor

Description

  • The BlockEvent stream currently uses the tari_broadcast_channel, this channel is bounded and does not clear when the receivers have read the events. As newly added and reorged blocks are distributed between services using this event stream, the channel can be memory intensive.
  • These changes replaced the tari_broadcast_channel used for the BlockEvent stream with a tokio broadcast channel. The Tokio broadcast channel has the benefit of removing the events from the channel as soon as all the receivers have read the events. This should minimise the amount of memory required.

Motivation and Context

The tari_broadcast_channel is memory intensive for sharing BlockEvents as it keeps the full set of events up to the specified bound limit even when the receivers have already processed the events. Using a tokio broadcast channel will result in less memory usage as all fully received events will be discarded without reaching the bound limit.

How Has This Been Tested?

Covered by existing tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Feature refactor (No new feature or functional changes, but performance or technical debt improvements)
  • New Tests
  • Documentation

Checklist:

  • I'm merging against the development branch.
  • I ran cargo-fmt --all before pushing.
  • I have squashed my commits into a single commit.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

philipr-za
philipr-za previously approved these changes May 20, 2020
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Looks good - but BlockEvent is too expensive to clone, so should be wrapped in an Arc when broadcasting.

…nts as it keeps the full set of events up to the specified bound limit even when the receivers have already processed the events. Using a tokio broadcast channel will result in less memory usage as all fully received events will be discarded without reaching the bound limit.

- The BlockEvent stream currently uses the tari_broadcast_channel, this channel is bounded and does not clear when the receivers have read the events. As newly added and reorged blocks are distributed between services using this event stream, the channel can be memory intensive.
- These changes replaced the tari_broadcast_channel used for the BlockEvent stream with a tokio broadcast channel. The Tokio broadcast channel has the benefit of removing the events from the channel as soon as all the receivers have read the events. This should minimise the amount of memory required.
@sdbondi sdbondi merged commit 702b51c into development May 20, 2020
@sdbondi sdbondi deleted the yuko-block_event_broadcast_channel branch May 22, 2020 11:46
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.

3 participants