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

Improve peak jitter calculations #776

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

gavv
Copy link
Member

@gavv gavv commented Aug 11, 2024

gh-712

  • 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.

QA: https://github.com/roc-streaming/qa/tree/main/manual/20241011_gh688_adaptive_latency

@gavv gavv added the ready for review Pull request can be reviewed label Aug 11, 2024
@gavv gavv added this to the next milestone Aug 11, 2024
@gavv gavv requested a review from baranovmv August 11, 2024 19:15
@gavv gavv mentioned this pull request Aug 11, 2024
3 tasks
//!
//! 3. Calculate moving maximum of the envelope's quantile across last `jitter_window`
//! samples. This is the resulting peak jitter.
struct JitterConfig {
Copy link
Member

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

Comment on lines 59 to 67
//! 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;
Copy link
Member

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".

Comment on lines 75 to 108
// 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)
// | ╲ | ╲_ | ╲_
// __| ╲_|╲__| ╲____| ╲____
Copy link
Member

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."

- 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.
@gavv
Copy link
Member Author

gavv commented Aug 13, 2024

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:

  1. rewrite peak jitter calculation, most likely use histogram of scaled samples (e.g. using logarithm) instead of peak detector + quantile
  2. re-do QA
  3. replace MovAggregate with two classes: MovMeanStd and MovMinMax

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.

@gavv gavv merged commit 21f8dfe into roc-streaming:develop Aug 13, 2024
45 checks passed
@gavv gavv deleted the feature/peak_jitter branch August 13, 2024 14:08
@gavv gavv removed the ready for review Pull request can be reviewed label Aug 13, 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.

2 participants