overload: allow creation of custom scaled timers#14077
overload: allow creation of custom scaled timers#14077mattklein123 merged 6 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alex Konradi <akonradi@google.com>
Add a method to the ThreadLocalOverloadState to support creating arbitrary scaled timers. This can be used by extension filters to scale their behavior in response to load without requiring a separate overload action registration. Filter factories can obtain a reference to the OverloadManager and use that to pass references to the thread-local state into the constructed filters, which can in turn be used to create scaled timers. Signed-off-by: Alex Konradi <akonradi@google.com>
Signed-off-by: Alex Konradi <akonradi@google.com>
Signed-off-by: Alex Konradi <akonradi@google.com>
| * Class that describes how to compute a minimum timeout given a maximum timeout value. It wraps | ||
| * ScaledMinimum and AbsoluteMinimum and provides a single computeMinimum() method. | ||
| */ | ||
| class ScaledTimerMinimum : private absl::variant<ScaledMinimum, AbsoluteMinimum> { |
There was a problem hiding this comment.
As a thought experiment, it would be interesting to see if this was easier to read if you just gave a struct with two values, and if one of the (abs min I think) is zero, just use the other one.
I'm also not sure if (a) speed matters for this or (b) whether this mechanism or the simpler one would be faster.
Another approach you might have gone for is inheritance for this. Because this is already implementing an abstract class I think this might be the fastest approach.
I understand what you have here, but it's worth just bouncing around whether a simpler rep and impl would be more maintainable.
There was a problem hiding this comment.
Re inheritance, it didn't seem worth the performance cost of virtual dispatch since the space of values is closed right now. #13800 changes ScaledTimerMinimum to wrap absl::variant instead of inheriting, which gets rid of some of the weirdness.
I don't think there is any performance improvement to be gained from the two-value version. It takes the same amount of space (two 8-byte words) and instead of jumping based on the value of the tag in the variant, it would jump based on the first word. As for why to keep the variant:
- it accurately models the intent of the class: there is exactly one value, and it can either be a scale factor or absolute duration
- it enforces invariants with types: instead of expecting maintainers to notice and abide by a convention, it uses the type system to enforce them
- it is amenable to change: if we added another type, the code wouldn't compile unless we were handling it in the right places (
absl::visit)
There was a problem hiding this comment.
yup, all true. What I was thinking about for inheritance was that you'd flatten it and exploit the fact that Timer is already pure, and not do another level of dispatch after that.
That might have been easier to suggest before you already defined ScaledRangeTimerManager::createTimer(variant_struct).
Anyway I think this is fine, but I thought that approach might be simpler & faster as there wouldn't need to be any visit step. But probably this level of speed optimization does not matter and I agree your intent is clear.
There was a problem hiding this comment.
Ah, I see: we'd effectively get rid of ScaledTimerMinimum as a separate class and then have ScaledMinimumTimer and AbsoluteMinimumTimer implementations of Timer. Yeah I agree, the very small speedup probably isn't worth it.
There was a problem hiding this comment.
no the main reason I thought of it is that it would be less code. absl::variant requires a lot of lines of code.
| * Class that describes how to compute a minimum timeout given a maximum timeout value. It wraps | ||
| * ScaledMinimum and AbsoluteMinimum and provides a single computeMinimum() method. | ||
| */ | ||
| class ScaledTimerMinimum : private absl::variant<ScaledMinimum, AbsoluteMinimum> { |
There was a problem hiding this comment.
yup, all true. What I was thinking about for inheritance was that you'd flatten it and exploit the fact that Timer is already pure, and not do another level of dispatch after that.
That might have been easier to suggest before you already defined ScaledRangeTimerManager::createTimer(variant_struct).
Anyway I think this is fine, but I thought that approach might be simpler & faster as there wouldn't need to be any visit step. But probably this level of speed optimization does not matter and I agree your intent is clear.
|
/retest |
|
Retrying Azure Pipelines: |
|
Looks like there is a flake in coverage you fell victim to: https://dev.azure.com/cncf/envoy/_build/results?buildId=57907&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881&l=14394 I don't see how this PR could've caused this but I'm guessing there's some non-deterministic monkey-business going on there somehow. Might as well tweak it in the coverage expectations file (not sure where that is but not hard to find). There's a clang-tidy error too: std::chrono::milliseconds probably needs an |
Signed-off-by: Alex Konradi <akonradi@google.com>
jmarantz
left a comment
There was a problem hiding this comment.
@envoyproxy/senior-maintainers
Signed-off-by: Alex Konradi <akonradi@google.com>
Add a method to the ThreadLocalOverloadState to support creating arbitrary scaled timers. This can be used by extension filters to scale their behavior in response to load without requiring a separate overload action registration. Filter factories can obtain a reference to the OverloadManager and use that to pass references to the thread-local state into the constructed filters, which can in turn be used to create scaled timers. Signed-off-by: Alex Konradi <akonradi@google.com> Signed-off-by: Qin Qin <qqin@google.com>
Commit Message: Support creating scaled timers in custom filters
Additional Description:
Add a method to the ThreadLocalOverloadState to support creating
arbitrary scaled timers. This can be used by extension filters to scale
their behavior in response to load without requiring a separate overload
action registration.
Filter factories can obtain a reference to the OverloadManager and use
that to pass references to the thread-local state into the constructed
filters, which can in turn be used to create scaled timers.
Risk Level: low
Testing: added unit tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a