diff --git a/include/envoy/event/BUILD b/include/envoy/event/BUILD index 790fb45f18398..a4c865a2d2255 100644 --- a/include/envoy/event/BUILD +++ b/include/envoy/event/BUILD @@ -44,7 +44,9 @@ envoy_cc_library( envoy_cc_library( name = "scaled_timer_minimum", hdrs = ["scaled_timer_minimum.h"], - deps = [], + deps = [ + "//source/common/common:interval_value", + ], ) envoy_cc_library( diff --git a/include/envoy/event/scaled_range_timer_manager.h b/include/envoy/event/scaled_range_timer_manager.h index c563050783526..7defc1b45be7d 100644 --- a/include/envoy/event/scaled_range_timer_manager.h +++ b/include/envoy/event/scaled_range_timer_manager.h @@ -4,6 +4,8 @@ #include "envoy/event/scaled_timer_minimum.h" #include "envoy/event/timer.h" +#include "absl/types/variant.h" + namespace Envoy { namespace Event { diff --git a/include/envoy/event/scaled_timer_minimum.h b/include/envoy/event/scaled_timer_minimum.h index e0f25aa4f2305..edf3b1777b7f0 100644 --- a/include/envoy/event/scaled_timer_minimum.h +++ b/include/envoy/event/scaled_timer_minimum.h @@ -2,6 +2,8 @@ #include +#include "common/common/interval_value.h" + #include "absl/types/variant.h" namespace Envoy { @@ -11,8 +13,8 @@ namespace Event { * Describes a minimum timer value that is equal to a scale factor applied to the maximum. */ struct ScaledMinimum { - explicit ScaledMinimum(double scale_factor) : scale_factor_(scale_factor) {} - const double scale_factor_; + explicit ScaledMinimum(UnitFloat scale_factor) : scale_factor_(scale_factor) {} + const UnitFloat scale_factor_; }; /** @@ -41,8 +43,8 @@ class ScaledTimerMinimum : private absl::variant struct Visitor { explicit Visitor(std::chrono::milliseconds value) : value_(value) {} std::chrono::milliseconds operator()(ScaledMinimum scale_factor) { - return std::chrono::duration_cast(scale_factor.scale_factor_ * - value_); + return std::chrono::duration_cast( + scale_factor.scale_factor_.value() * value_); } std::chrono::milliseconds operator()(AbsoluteMinimum absolute_value) { return absolute_value.value_; diff --git a/include/envoy/server/overload/BUILD b/include/envoy/server/overload/BUILD index 89ae23c63c2a5..f6f4a90933e99 100644 --- a/include/envoy/server/overload/BUILD +++ b/include/envoy/server/overload/BUILD @@ -26,5 +26,6 @@ envoy_cc_library( "//include/envoy/event:scaled_timer_minimum", "//include/envoy/event:timer_interface", "//include/envoy/thread_local:thread_local_object", + "//source/common/common:interval_value", ], ) diff --git a/include/envoy/server/overload/thread_local_overload_state.h b/include/envoy/server/overload/thread_local_overload_state.h index 8d95a42b01323..5a4248b6e34a7 100644 --- a/include/envoy/server/overload/thread_local_overload_state.h +++ b/include/envoy/server/overload/thread_local_overload_state.h @@ -8,6 +8,8 @@ #include "envoy/event/timer.h" #include "envoy/thread_local/thread_local_object.h" +#include "common/common/interval_value.h" + namespace Envoy { namespace Server { @@ -20,18 +22,17 @@ namespace Server { */ class OverloadActionState { public: - static constexpr OverloadActionState inactive() { return OverloadActionState(0); } + static constexpr OverloadActionState inactive() { return OverloadActionState(UnitFloat::min()); } - static constexpr OverloadActionState saturated() { return OverloadActionState(1.0); } + static constexpr OverloadActionState saturated() { return OverloadActionState(UnitFloat::max()); } - explicit constexpr OverloadActionState(float value) - : action_value_(std::min(1.0f, std::max(0.0f, value))) {} + explicit constexpr OverloadActionState(UnitFloat value) : action_value_(value) {} - float value() const { return action_value_; } - bool isSaturated() const { return action_value_ == 1; } + float value() const { return action_value_.value(); } + bool isSaturated() const { return action_value_.value() == UnitFloat::max().value(); } private: - float action_value_; + UnitFloat action_value_; }; /** diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 89e41ae0c7b1d..f5632b8fee279 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -116,6 +116,11 @@ envoy_cc_library( deps = [":utility_lib"], ) +envoy_cc_library( + name = "interval_value", + hdrs = ["interval_value.h"], +) + envoy_cc_library( name = "linked_object", hdrs = ["linked_object.h"], diff --git a/source/common/common/interval_value.h b/source/common/common/interval_value.h new file mode 100644 index 0000000000000..454c493e0d25b --- /dev/null +++ b/source/common/common/interval_value.h @@ -0,0 +1,37 @@ +#pragma once + +#include + +namespace Envoy { + +// Template helper type that represents a closed interval with the given minimum and maximum values. +template struct Interval { + static_assert(MinValue <= MaxValue, "min must be <= max"); + static constexpr T min_value = MinValue; + static constexpr T max_value = MaxValue; +}; + +// Utility type that represents a value of type T in the given interval. +template class ClosedIntervalValue { +public: + static constexpr ClosedIntervalValue min() { return ClosedIntervalValue(Interval::min_value); } + static constexpr ClosedIntervalValue max() { return ClosedIntervalValue(Interval::max_value); } + + constexpr explicit ClosedIntervalValue(T value) + : value_(std::max(Interval::min_value, std::min(Interval::max_value, value))) {} + + T value() const { return value_; } + +private: + T value_; +}; + +// C++17 doesn't allow templating on floating point values, otherwise that's +// what we should do here instead of relying on a int => float implicit +// conversion. TODO(akonradi): when Envoy is using C++20, switch these template +// parameters to floats. + +// Floating point value in the range [0, 1]. +using UnitFloat = ClosedIntervalValue>; + +} // namespace Envoy diff --git a/source/common/event/scaled_range_timer_manager_impl.cc b/source/common/event/scaled_range_timer_manager_impl.cc index 3959976846d85..6aab81486929c 100644 --- a/source/common/event/scaled_range_timer_manager_impl.cc +++ b/source/common/event/scaled_range_timer_manager_impl.cc @@ -152,7 +152,7 @@ TimerPtr ScaledRangeTimerManagerImpl::createTimer(ScaledTimerMinimum minimum, Ti void ScaledRangeTimerManagerImpl::setScaleFactor(double scale_factor) { const MonotonicTime now = dispatcher_.approximateMonotonicTime(); - scale_factor_ = DurationScaleFactor(scale_factor); + scale_factor_ = UnitFloat(scale_factor); for (auto& queue : queues_) { resetQueueTimer(*queue, now); } @@ -171,12 +171,9 @@ ScaledRangeTimerManagerImpl::ScalingTimerHandle::ScalingTimerHandle(Queue& queue Queue::Iterator iterator) : queue_(queue), iterator_(iterator) {} -ScaledRangeTimerManagerImpl::DurationScaleFactor::DurationScaleFactor(double value) - : value_(std::max(0.0, std::min(value, 1.0))) {} - MonotonicTime ScaledRangeTimerManagerImpl::computeTriggerTime(const Queue::Item& item, std::chrono::milliseconds duration, - DurationScaleFactor scale_factor) { + UnitFloat scale_factor) { return item.active_time_ + std::chrono::duration_cast(duration * scale_factor.value()); } diff --git a/source/common/event/scaled_range_timer_manager_impl.h b/source/common/event/scaled_range_timer_manager_impl.h index d1f6624c3bf10..f9bcc7242b7d1 100644 --- a/source/common/event/scaled_range_timer_manager_impl.h +++ b/source/common/event/scaled_range_timer_manager_impl.h @@ -74,16 +74,6 @@ class ScaledRangeTimerManagerImpl : public ScaledRangeTimerManager { Queue::Iterator iterator_; }; - // A simple wrapper around a float that ensures value() is sane (in the range [0, 1]). - class DurationScaleFactor { - public: - DurationScaleFactor(double value); - double value() const { return value_; } - - private: - double value_; - }; - struct Hash { // Magic declaration to allow heterogeneous lookup. using is_transparent = void; // NOLINT(readability-identifier-naming) @@ -113,7 +103,7 @@ class ScaledRangeTimerManagerImpl : public ScaledRangeTimerManager { static MonotonicTime computeTriggerTime(const Queue::Item& item, std::chrono::milliseconds duration, - DurationScaleFactor scale_factor); + UnitFloat scale_factor); ScalingTimerHandle activateTimer(std::chrono::milliseconds duration, RangeTimerImpl& timer); @@ -124,7 +114,7 @@ class ScaledRangeTimerManagerImpl : public ScaledRangeTimerManager { void onQueueTimerFired(Queue& queue); Dispatcher& dispatcher_; - DurationScaleFactor scale_factor_; + UnitFloat scale_factor_; absl::flat_hash_set, Hash, Eq> queues_; }; diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index 22fea559efa7a..2ec86122eaa23 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -31,7 +31,7 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { const NamedOverloadActionSymbolTable& action_symbol_table, const absl::flat_hash_map& timer_minimums) : action_symbol_table_(action_symbol_table), timer_minimums_(timer_minimums), - actions_(action_symbol_table.size(), OverloadActionState(0)), + actions_(action_symbol_table.size(), OverloadActionState(UnitFloat::min())), scaled_timer_action_(action_symbol_table.lookup(OverloadActionNames::get().ReduceTimeouts)), scaled_timer_manager_(std::move(scaled_timer_manager)) {} @@ -46,8 +46,9 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { Event::TimerCb callback) override { auto minimum_it = timer_minimums_.find(timer_type); const Event::ScaledTimerMinimum minimum = - minimum_it != timer_minimums_.end() ? minimum_it->second - : Event::ScaledTimerMinimum(Event::ScaledMinimum(1.0)); + minimum_it != timer_minimums_.end() + ? minimum_it->second + : Event::ScaledTimerMinimum(Event::ScaledMinimum(UnitFloat::max())); return scaled_timer_manager_->createTimer(minimum, std::move(callback)); } @@ -72,7 +73,7 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState { const Event::ScaledRangeTimerManagerPtr scaled_timer_manager_; }; -const OverloadActionState ThreadLocalOverloadStateImpl::always_inactive_{0.0}; +const OverloadActionState ThreadLocalOverloadStateImpl::always_inactive_{UnitFloat::min()}; namespace { @@ -113,8 +114,8 @@ class ScaledTriggerImpl final : public OverloadAction::Trigger { } else if (value >= saturated_threshold_) { state_ = OverloadActionState::saturated(); } else { - state_ = OverloadActionState((value - scaling_threshold_) / - (saturated_threshold_ - scaling_threshold_)); + state_ = OverloadActionState( + UnitFloat((value - scaling_threshold_) / (saturated_threshold_ - scaling_threshold_))); } return state_.value() != old_state.value(); } @@ -169,7 +170,7 @@ parseTimerMinimums(const ProtobufWkt::Any& typed_config, ? Event::ScaledTimerMinimum(Event::AbsoluteMinimum(std::chrono::milliseconds( DurationUtil::durationToMilliseconds(scale_timer.min_timeout())))) : Event::ScaledTimerMinimum( - Event::ScaledMinimum(scale_timer.min_scale().value() / 100.0)); + Event::ScaledMinimum(UnitFloat(scale_timer.min_scale().value() / 100.0))); auto [_, inserted] = timer_map.insert(std::make_pair(timer_type, minimum)); if (!inserted) { diff --git a/test/common/common/BUILD b/test/common/common/BUILD index c002da716b0a6..de9cfd7427e48 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -354,3 +354,9 @@ envoy_cc_test( "//source/common/http:status_lib", ], ) + +envoy_cc_test( + name = "interval_value_test", + srcs = ["interval_value_test.cc"], + deps = ["//source/common/common:interval_value"], +) diff --git a/test/common/common/interval_value_test.cc b/test/common/common/interval_value_test.cc new file mode 100644 index 0000000000000..41a6c3fbc0778 --- /dev/null +++ b/test/common/common/interval_value_test.cc @@ -0,0 +1,31 @@ +#include "common/common/interval_value.h" + +#include "gtest/gtest.h" + +namespace Envoy { + +using TestInterval = Interval; + +TEST(IntervalTest, Max) { EXPECT_EQ(TestInterval::max_value, 10); } + +TEST(IntervalTest, Min) { EXPECT_EQ(TestInterval::min_value, 5); } + +using LongIntervalValue = ClosedIntervalValue; + +TEST(ClosedIntervalValueTest, Max) { EXPECT_EQ(LongIntervalValue::max().value(), 10); } + +TEST(ClosedIntervalValueTest, Min) { EXPECT_EQ(LongIntervalValue::min().value(), 5); } + +TEST(ClosedIntervalValueTest, FromBelowMin) { EXPECT_EQ(LongIntervalValue(3).value(), 5); } + +TEST(ClosedIntervalValueTest, FromAboveMax) { EXPECT_EQ(LongIntervalValue(20).value(), 10); } + +using FloatIntervalValue = ClosedIntervalValue; + +TEST(ClosedIntervalValueTest, MixIntAndFloat) { + EXPECT_EQ(FloatIntervalValue(0).value(), 5.0f); + EXPECT_EQ(FloatIntervalValue(5).value(), 5.0f); + EXPECT_EQ(FloatIntervalValue(20).value(), 10.0f); +} + +} // namespace Envoy diff --git a/test/common/event/scaled_range_timer_manager_impl_test.cc b/test/common/event/scaled_range_timer_manager_impl_test.cc index 2e0afed349b19..824a285ee4f61 100644 --- a/test/common/event/scaled_range_timer_manager_impl_test.cc +++ b/test/common/event/scaled_range_timer_manager_impl_test.cc @@ -66,7 +66,7 @@ TEST_F(ScaledRangeTimerManagerTest, CreateAndDestroyTimer) { { MockFunction callback; - auto timer = manager.createTimer(ScaledMinimum(1.0), callback.AsStdFunction()); + auto timer = manager.createTimer(ScaledMinimum(UnitFloat(1.0)), callback.AsStdFunction()); } } @@ -74,7 +74,7 @@ TEST_F(ScaledRangeTimerManagerTest, CreateSingleScaledTimer) { ScaledRangeTimerManagerImpl manager(dispatcher_); MockFunction callback; - auto timer = manager.createTimer(ScaledMinimum(0.5), callback.AsStdFunction()); + auto timer = manager.createTimer(ScaledMinimum(UnitFloat(0.5)), callback.AsStdFunction()); timer->enableTimer(std::chrono::seconds(10)); EXPECT_TRUE(timer->enabled()); @@ -110,7 +110,7 @@ TEST_F(ScaledRangeTimerManagerTest, DisableWhileDisabled) { ScaledRangeTimerManagerImpl manager(dispatcher_); MockFunction callback; - auto timer = manager.createTimer(ScaledMinimum(1.0), callback.AsStdFunction()); + auto timer = manager.createTimer(ScaledMinimum(UnitFloat(1.0)), callback.AsStdFunction()); EXPECT_FALSE(timer->enabled()); timer->disableTimer(); @@ -155,7 +155,7 @@ TEST_F(ScaledRangeTimerManagerTest, DisableWithZeroMinTime) { ScaledRangeTimerManagerImpl manager(dispatcher_); MockFunction callback; - auto timer = manager.createTimer(ScaledMinimum(0.0), callback.AsStdFunction()); + auto timer = manager.createTimer(ScaledMinimum(UnitFloat(0.0)), callback.AsStdFunction()); timer->enableTimer(std::chrono::seconds(100)); @@ -319,7 +319,7 @@ TEST_F(ScaledRangeTimerManagerTest, SingleTimerSameMinMax) { ScaledRangeTimerManagerImpl manager(dispatcher_); MockFunction callback; - auto timer = manager.createTimer(ScaledMinimum(1.0), callback.AsStdFunction()); + auto timer = manager.createTimer(ScaledMinimum(UnitFloat(1.0)), callback.AsStdFunction()); EXPECT_CALL(callback, Call()); timer->enableTimer(std::chrono::seconds(1)); @@ -333,7 +333,7 @@ TEST_F(ScaledRangeTimerManagerTest, ScaledMinimumFactorGreaterThan1) { // If the minimum scale factor is > 1, it should be treated as if it was 1. MockFunction callback; - auto timer = manager.createTimer(ScaledMinimum(2), callback.AsStdFunction()); + auto timer = manager.createTimer(ScaledMinimum(UnitFloat(2)), callback.AsStdFunction()); timer->enableTimer(std::chrono::seconds(10)); EXPECT_CALL(callback, Call); diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 57bcd5d0697e7..fc58ff2277f05 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -2070,7 +2070,7 @@ TEST(HttpConnectionManagerTracingStatsTest, verifyTracingStats) { } TEST_F(HttpConnectionManagerImplTest, NoNewStreamWhenOverloaded) { - Server::OverloadActionState stop_accepting_requests = Server::OverloadActionState(0.8); + Server::OverloadActionState stop_accepting_requests(UnitFloat(0.8)); ON_CALL(overload_manager_.overload_state_, getState(Server::OverloadActionNames::get().StopAcceptingRequests)) .WillByDefault(ReturnRef(stop_accepting_requests)); @@ -2095,7 +2095,7 @@ TEST_F(HttpConnectionManagerImplTest, NoNewStreamWhenOverloaded) { } TEST_F(HttpConnectionManagerImplTest, DisableHttp1KeepAliveWhenOverloaded) { - Server::OverloadActionState disable_http_keep_alive = Server::OverloadActionState(0.8); + Server::OverloadActionState disable_http_keep_alive(UnitFloat(0.8)); ON_CALL(overload_manager_.overload_state_, getState(Server::OverloadActionNames::get().DisableHttpKeepAlive)) .WillByDefault(ReturnRef(disable_http_keep_alive));