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

feat(spooler): Implement backpressure mechanism based on atomics #4040

Merged
merged 9 commits into from
Sep 17, 2024

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Sep 17, 2024

This PR adds a new, simpler backpressure mechanism that relies on a shared AtomicBool flag. The rationale behind this implementation is that we observed tokio oneshot channels can experience spurious failures, causing the Receiver to return Poll::Pending, which skews the buffer towards more writes instead of reads, resulting in disk spooling and a consequent system slowdown. With AtomicBool, we aim to achieve better performance since the flag update should be relatively fast.

#skip-changelog

@iambriccardo iambriccardo marked this pull request as ready for review September 17, 2024 09:52
@iambriccardo iambriccardo requested a review from a team as a code owner September 17, 2024 09:52
Copy link
Member

@jjbayer jjbayer 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 I think we should not rely on the select to cancel the future.

@@ -169,6 +173,11 @@ impl EnvelopeBufferService {
tokio::time::sleep(self.sleep).await;
}

// In case the project cache is not ready, we don't want to attempt popping.
if !self.project_cache_ready.load(Ordering::Relaxed) {
return future::pending().await;
Copy link
Member

Choose a reason for hiding this comment

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

can we do a sleep for DEFAULT_SLEEP here? If Relay is in a mode where it does not receive any messages, this might block forever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

Am I understanding this correctly: The EnvelopeBufferService checks the flag and if it's set (i.e. ready), it sends an envelope to the ProjectCacheService and flips the flag to false. The ProjectCacheService flips the flag back to true once it has processed what it was sent to signal that it's ready for more work?

@iambriccardo
Copy link
Member Author

Am I understanding this correctly: The EnvelopeBufferService checks the flag and if it's set (i.e. ready), it sends an envelope to the ProjectCacheService and flips the flag to false. The ProjectCacheService flips the flag back to true once it has processed what it was sent to signal that it's ready for more work?

Yes, correct!

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

As discussed offline, a tokio watch would be suited for this task, but this PR is a good experiment to test whether back pressure works as we expect.

@iambriccardo iambriccardo merged commit 0198a23 into master Sep 17, 2024
23 checks passed
@iambriccardo iambriccardo deleted the riccardo/feat/backpressure-bool branch September 17, 2024 13:10
iambriccardo added a commit that referenced this pull request Sep 18, 2024
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