-
Notifications
You must be signed in to change notification settings - Fork 13
feat: Add uniflight crate for duplicate request coalescing #118
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
base: main
Are you sure you want to change the base?
Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
========================================
Coverage 100.0% 100.0%
========================================
Files 105 106 +1
Lines 6767 6902 +135
========================================
+ Hits 6767 6902 +135 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I like the name :-) |
geeknoid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logo.png file isn't rendering for me. Is this a valid png file? I tried even going to your repo & branch, and GitHub won't show the image to me.
|
@geeknoid I hadn't uploaded the logo yet. I just added it |
Co-authored-by: Martin Taillefer <geeknoid@users.noreply.github.com>
Co-authored-by: Martin Taillefer <geeknoid@users.noreply.github.com>
|
|
||
| [package.metadata.cargo_check_external_types] | ||
| allowed_external_types = [ | ||
| "thread_aware::cell::builtin::PerProcess", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to reexport these?
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| //! Coalesces duplicate async tasks into a single execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using this line as the description in Cargo.toml and in the repo-level README file.
| //! `Merger` handles task cancellation and panics gracefully: | ||
| //! | ||
| //! - If the leader task is cancelled or dropped, a follower becomes the new leader | ||
| //! - If the leader task panics, a follower becomes the new leader and executes its work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong. Shouldn't the panic propagate to all followers?
| //! | ||
| //! - If the leader task is cancelled or dropped, a follower becomes the new leader | ||
| //! - If the leader task panics, a follower becomes the new leader and executes its work | ||
| //! - Followers that join before the leader completes receive the cached result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"receive the cached result" confuses me. Can we just say "receive the value the leader returns"?
| //! | ||
| //! # Memory Management | ||
| //! | ||
| //! Completed entries are automatically removed from the internal map when the last caller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. Seems like having a TTL here might be desirable? Keep an entry around for N msec in case someone asks for the result? This can turn into a simple short-lived cache.
The cargo-rank app I'm working on issues sometimes thousands of requests over the course of a few minutes as it runs. I am planning to adopt this crate once it's available. But if I want to truly be efficient, I'm going to need to implement a cache in front of uniflight to avoid issuing the same request multiple times over the course of those minutes. if I could tell uniflight to hold on to the keys & values for a bit, then it could totally avoid me doing the work N times.
But this leads to Hmmm #2. Is uniflight just a cache with an oddball TTL? Instead of holding on to items for a specific time period, it throws out cached data is nobody is actively waiting for it. Why should I use uniflight instead of a cache of tasks? I can achieve the same semantics by setting a TTL of 0, and get more flexible semantics if I increase the TTL.
| { | ||
| fn default() -> Self { | ||
| Self { | ||
| inner: TaArc::new(DashMap::new), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably should override the hasher, or allow the caller to override the hasher. The default hasher used by DashMap is the default system hasher which is really slow. Consider using ahash instead, or one of the other many fast hashers.
| where | ||
| K: Hash + Eq + Send + Sync + 'static, | ||
| T: Clone + Send + Sync + 'static, | ||
| S: Strategy + Send + Sync, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| S: Strategy + Send + Sync, | |
| S: Strategy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably applies in other places too.
| impl<K, T, S> Default for Merger<K, T, S> | ||
| where | ||
| K: Hash + Eq + Send + Sync + 'static, | ||
| T: Clone + Send + Sync + 'static, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| T: Clone + Send + Sync + 'static, | |
| T: Send + Sync + 'static, |
| impl<K, T, S> Merger<K, T, S> | ||
| where | ||
| K: Hash + Eq + Send + Sync + 'static, | ||
| T: Clone + Send + Sync + 'static, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| T: Clone + Send + Sync + 'static, | |
| T: Send + Sync + 'static, |
| where | ||
| K: Hash + Eq + Send + Sync + 'static, | ||
| T: Clone + Send + Sync + 'static, | ||
| S: Strategy + Send + Sync, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| S: Strategy + Send + Sync, | |
| S: Send + Sync, |
| impl<K, T> Merger<K, T, PerProcess> | ||
| where | ||
| K: Hash + Eq + Send + Sync + 'static, | ||
| T: Clone + Send + Sync + 'static, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| T: Clone + Send + Sync + 'static, | |
| T: Send + Sync + 'static, |
| impl<K, T> Merger<K, T, PerNuma> | ||
| where | ||
| K: Hash + Eq + Send + Sync + 'static, | ||
| T: Clone + Send + Sync + 'static, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| T: Clone + Send + Sync + 'static, | |
| T: Send + Sync + 'static, |
| impl<K, T> Merger<K, T, PerCore> | ||
| where | ||
| K: Hash + Eq + Send + Sync + 'static, | ||
| T: Clone + Send + Sync + 'static, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| T: Clone + Send + Sync + 'static, | |
| T: Send + Sync + 'static, |
|
|
||
| impl<K, T, S: Strategy> Merger<K, T, S> | ||
| where | ||
| K: Hash + Eq, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this trait bound, it's not needed.
| impl<K, T, S> Merger<K, T, S> | ||
| where | ||
| K: Hash + Eq + Send + Sync, | ||
| T: Clone + Send + Sync, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove Clone from here, and put it as a bound on the execute method instead.
Adds
uniflight, a crate for coalescing duplicate async tasks into a single execution. When multiple tasks request the same work (identified by a key), only the leader performs the work while followers wait and receive a clone of the result.