-
-
Notifications
You must be signed in to change notification settings - Fork 212
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
Improve peak jitter calculations #776
Conversation
72de021
to
28e74a0
Compare
//! | ||
//! 3. Calculate moving maximum of the envelope's quantile across last `jitter_window` | ||
//! samples. This is the resulting peak jitter. | ||
struct JitterConfig { |
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.
At first glance it looks like we configuring Jitter itself. I'd rather use JitterMeterConfig
//! Exponent coefficient of capacitor resistance used in jitter envelope. | ||
//! @note | ||
//! Capacitor discharge speed is (peak ^ exp) * coeff, where `peak` is | ||
//! the jitter peak size relative to the average jitter, `exp` is | ||
//! `envelope_resistance_exponent`, and `coeff` is `envelope_resistance_coeff`. | ||
//! @remarks | ||
//! Increase this value to make impact to the peak jitter of high spikes much | ||
//! stronger than impact of low spikes. | ||
double envelope_resistance_exponent; |
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.
Capacitor discharge speed is ...
Looks like it actually is a charge value itself, not "speed".
// This function calculates jitter envelope using a peak detector with capacitor. | ||
// | ||
// The quantile of jitter envelope is used as the value for `peak_jitter` metric. | ||
// LatencyTuner selects target latency based on its value. We want find lowest | ||
// possible peak jitter and target latency that are safe (don't cause disruptions). | ||
// | ||
// The function tries to achieve two goals: | ||
// | ||
// - The quantile of envelope (e.g. 90% of values) should be above regular repeating | ||
// spikes, typical for wireless networks, and should ignore occasional exceptions | ||
// if they're not too high and not too frequent. | ||
// | ||
// - The quantile of envelope should be however increased if occasional spike is | ||
// really high, which is often a predictor of increasing network load | ||
// (i.e. if spike is abnormally high, chances are that more high spikes follows). | ||
// | ||
// The role of the capacitor is to amplify the impact of jitter spikes of certain | ||
// kinds. Without it, spikes would be too thin to be reliably detected by quantile. | ||
// | ||
// Typical jitter envelope before applying capacitor: | ||
// | ||
// ------------------------------------- maximum (too high) | ||
// |╲ | ||
// || |╲ |╲ | ||
// --||----------||--------||----------- quantile (too low) | ||
// __||______|╲__||__|╲____||__|╲____ | ||
// | ||
// And after applying capacitor: | ||
// | ||
// |╲_ | ||
// --| |_-------|╲_-------|╲----------- quantile (good) | ||
// | ╲ | ╲_ | ╲_ | ||
// __| ╲_|╲__| ╲____| ╲____ |
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.
a peak detector with capacitor.
-> a model of a leaky peak detector.
I suppose any kind of a peak detector includes a capacitor in their circuit, though this sentence seems like it refers to some particular type of a peak detector, which you expect a reader to be aware of. I'd suggest
The role of the capacitor is to amplify the impact of jitter spikes
-> "A leaky peak detector takes immediate peaks and mimicking a leakage process when immediate values of jitter are lower than stored one."
28e74a0
to
f1c5b24
Compare
- Extract JitterMeter class. - Remove `min_jitter` metric (it is almost always zero or very close to it). - Replace `max_jitter` with `peak_jitter`, which is similar to maximum, but tries to exclude harmless spikes to reduce latency. See comments in JitterMeter for details on the algorithm.
As discussed with Mike, this algorithm will be only temporary solution not to be released, as we're unable to conclude if it's generic enough, especially magic numbers that it uses. Before releasing 0.5, we should do 2 changes:
Mike, could you create a task for (1)+(2)? No need to make it super specific, but would be nice if you can share what data you would like to see in QA reports (given that we still use current tooling) and some ideas for the new algorithm. |
f1c5b24
to
2c84c4f
Compare
gh-712
min_jitter
metric (it is almost always zero or very close to it).max_jitter
withpeak_jitter
, which is similar to maximum, but tries to exclude harmless spikes to reduce latency.See comments in JitterMeter for details on the algorithm.
QA: https://github.com/roc-streaming/qa/tree/main/manual/20241011_gh688_adaptive_latency