cleanup: replace ad-hoc [0, 1] value types with UnitFloat

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(

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 {
 

include/envoy/event/scaled_timer_minimum.h

@@ -2,6 +2,8 @@
 
 #include <chrono>
 
+#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<ScaledMinimum, AbsoluteMinimum>
     struct Visitor {
       explicit Visitor(std::chrono::milliseconds value) : value_(value) {}
       std::chrono::milliseconds operator()(ScaledMinimum scale_factor) {
-        return std::chrono::duration_cast<std::chrono::milliseconds>(scale_factor.scale_factor_ *
-                                                                     value_);
+        return std::chrono::duration_cast<std::chrono::milliseconds>(
+            scale_factor.scale_factor_.value() * value_);
       }
       std::chrono::milliseconds operator()(AbsoluteMinimum absolute_value) {
         return absolute_value.value_;

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",
     ],
 )

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_;
 };
 
 /**

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"],

source/common/common/interval_value.h

@@ -0,0 +1,37 @@
+#pragma once
+
+#include <algorithm>
+
+namespace Envoy {
+
+// Template helper type that represents a closed interval with the given minimum and maximum values.
+template <typename T, T MinValue, T MaxValue> 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 <typename T, typename Interval> 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<T>(Interval::min_value, std::min<T>(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<float, Interval<int, 0, 1>>;
+
+} // namespace Envoy

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<MonotonicTime::duration>(duration * scale_factor.value());
 }

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<std::unique_ptr<Queue>, Hash, Eq> queues_;
 };
 

source/server/overload_manager_impl.cc

@@ -31,7 +31,7 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState {
       const NamedOverloadActionSymbolTable& action_symbol_table,
       const absl::flat_hash_map<OverloadTimerType, Event::ScaledTimerMinimum>& 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) {

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"],
+)

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<int, 5, 10>;
+
+TEST(IntervalTest, Max) { EXPECT_EQ(TestInterval::max_value, 10); }
+
+TEST(IntervalTest, Min) { EXPECT_EQ(TestInterval::min_value, 5); }
+
+using LongIntervalValue = ClosedIntervalValue<long, TestInterval>;
+
+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<float, TestInterval>;
+
+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

test/common/event/scaled_range_timer_manager_impl_test.cc

@@ -66,15 +66,15 @@ TEST_F(ScaledRangeTimerManagerTest, CreateAndDestroyTimer) {
 
   {
     MockFunction<TimerCb> callback;
-    auto timer = manager.createTimer(ScaledMinimum(1.0), callback.AsStdFunction());
+    auto timer = manager.createTimer(ScaledMinimum(UnitFloat(1.0)), callback.AsStdFunction());
   }
 }
 
 TEST_F(ScaledRangeTimerManagerTest, CreateSingleScaledTimer) {
   ScaledRangeTimerManagerImpl manager(dispatcher_);
 
   MockFunction<TimerCb> 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<TimerCb> 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<TimerCb> 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<TimerCb> 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<TimerCb> 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);

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));