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

perf: Use RTT variance instead of a fixed threshold #3671

Closed
wants to merge 2 commits into from

Conversation

bruceg
Copy link
Member

@bruceg bruceg commented Sep 1, 2020

Instead of using a fixed threshold, this calculates the standard deviation of the EWMA of the past RTT measurements, and uses a multiple of that as the threshold range.

Bruce Guenter added 2 commits September 1, 2020 15:46
Instead of using a fixed threshold, this calculates the standard
deviation of the EWMA of the past RTT measurements, and uses a multiple
of that as the threshold range.

Signed-off-by: Bruce Guenter <bruce@timber.io>
This also adds an internal option for a custom EWMA alpha.

Signed-off-by: Bruce Guenter <bruce@timber.io>
@bruceg bruceg added type: enhancement A value-adding code change that enhances its existing functionality. domain: networking Anything related to Vector's networking domain: sinks Anything related to the Vector's sinks domain: performance Anything related to Vector's performance labels Sep 1, 2020
@bruceg bruceg requested a review from jszwedko September 1, 2020 21:55
@bruceg bruceg self-assigned this Sep 1, 2020
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

🎉

I'll try this out with the tests we have so far.

let variance = self.variance.unwrap_or(0.0);
(
point * self.alpha + avg * (1.0 - self.alpha),
Some((1.0 - self.alpha) * (variance + self.alpha * delta * delta)),
Copy link
Member

Choose a reason for hiding this comment

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

Would you be able to explain a bit what this is doing?

Copy link
Member

Choose a reason for hiding this comment

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

(specifically the variance bit of the calculation)

@jszwedko
Copy link
Member

jszwedko commented Sep 2, 2020

I note that I see:

warning: method is never used: `new`
   --> src/sinks/util/auto_concurrency/controller.rs:266:5
    |
266 |     fn new(alpha: f64) -> Self {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: 1 warning emitted

When building.

I reran the tests (including the added "knee behavior" tests).

Some observations:

  • It seems like it does better in the small variance case, but actually appears worse in the medium/large variance cases
  • It seems to do better for the knee case with a higher slope

Before (424ea5e):

all

After (6670a0e):

all

I'm realizing it'd probably be useful to plot before/after on the same graphs (specifically the throughput) to make it easier to see differences. I opened vectordotdev/http_test_server#4 to that end.

@bruceg
Copy link
Member Author

bruceg commented Oct 19, 2020

Since this doesn't seem to have helped any, at least with our simulations, and needs some work to be usable, I will close this for now and see what happens in real deployment situations.

@bruceg bruceg closed this Oct 19, 2020
@binarylogic binarylogic deleted the auto-concurrency-handle-variance branch January 19, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: networking Anything related to Vector's networking domain: performance Anything related to Vector's performance domain: sinks Anything related to the Vector's sinks type: enhancement A value-adding code change that enhances its existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants