refactor: use unitfloat in more places (#14396)

Commit Message: Use UnitFloat in place of float in more locations Additional Description: UnitFloat represents a floating point value that is guaranteed to be in the range [0, 1]. Use it in place of floats that also have the same expectation in OverloadActionState and connection listeners. This PR introduces no functional changes. Risk Level: low Testing: ran affected tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Alex Konradi <[email protected]>
envoyproxy · Jan 13, 2021 · a376d36 · a376d36
1 parent ad7d36f
commit a376d36
Show file tree

Hide file tree

Showing 25 changed files with 102 additions and 58 deletions.
diff --git a/include/envoy/common/BUILD b/include/envoy/common/BUILD
@@ -38,6 +38,7 @@ envoy_cc_library(
 envoy_cc_library(
     name = "random_generator_interface",
     hdrs = ["random_generator.h"],
+    deps = ["//source/common/common:interval_value"],
 )
 
 envoy_cc_library(

diff --git a/include/envoy/common/random_generator.h b/include/envoy/common/random_generator.h
@@ -6,6 +6,8 @@
 
 #include "envoy/common/pure.h"
 
+#include "common/common/interval_value.h"
+
 namespace Envoy {
 namespace Random {
 
@@ -50,13 +52,13 @@ class RandomGenerator {
   /**
    * @return a random boolean value, with probability `p` equaling true.
    */
-  bool bernoulli(float p) {
-    if (p <= 0) {
+  bool bernoulli(UnitFloat p) {
+    if (p == UnitFloat::min()) {
       return false;
-    } else if (p >= 1) {
+    } else if (p == UnitFloat::max()) {
       return true;
     }
-    return random() < static_cast<result_type>(p * static_cast<float>(max()));
+    return random() < static_cast<result_type>(p.value() * static_cast<float>(max()));
   }
 };
 

diff --git 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 "common/common/interval_value.h"
+
 #include "absl/types/variant.h"
 
 namespace Envoy {
@@ -36,7 +38,7 @@ class ScaledRangeTimerManager {
    * factor of 0.5 causes firing halfway between min and max, and a factor of 1 causes firing at
    * max.
    */
-  virtual void setScaleFactor(double scale_factor) PURE;
+  virtual void setScaleFactor(UnitFloat scale_factor) PURE;
 };
 
 using ScaledRangeTimerManagerPtr = std::unique_ptr<ScaledRangeTimerManager>;

diff --git a/include/envoy/network/BUILD b/include/envoy/network/BUILD
@@ -38,6 +38,7 @@ envoy_cc_library(
         ":listen_socket_interface",
         ":listener_interface",
         "//include/envoy/ssl:context_interface",
+        "//source/common/common:interval_value",
     ],
 )
 
@@ -173,6 +174,7 @@ envoy_cc_library(
         "//include/envoy/common:resource_interface",
         "//include/envoy/init:manager_interface",
         "//include/envoy/stats:stats_interface",
+        "//source/common/common:interval_value",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
     ],
 )

diff --git a/include/envoy/network/connection_handler.h b/include/envoy/network/connection_handler.h
@@ -9,6 +9,8 @@
 #include "envoy/network/listener.h"
 #include "envoy/ssl/context.h"
 
+#include "common/common/interval_value.h"
+
 namespace Envoy {
 namespace Network {
 
@@ -98,7 +100,7 @@ class ConnectionHandler {
    * Set the fraction of connections the listeners should reject.
    * @param reject_fraction a value between 0 (reject none) and 1 (reject all).
    */
-  virtual void setListenerRejectFraction(float reject_fraction) PURE;
+  virtual void setListenerRejectFraction(UnitFloat reject_fraction) PURE;
 
   /**
    * @return the stat prefix used for per-handler stats.

diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h
@@ -16,6 +16,8 @@
 #include "envoy/network/udp_packet_writer_handler.h"
 #include "envoy/stats/scope.h"
 
+#include "common/common/interval_value.h"
+
 namespace Envoy {
 namespace Network {
 
@@ -332,7 +334,7 @@ class Listener {
    * Set the fraction of incoming connections that will be closed immediately
    * after being opened.
    */
-  virtual void setRejectFraction(float reject_fraction) PURE;
+  virtual void setRejectFraction(UnitFloat reject_fraction) PURE;
 };
 
 using ListenerPtr = std::unique_ptr<Listener>;

diff --git a/include/envoy/server/overload/thread_local_overload_state.h b/include/envoy/server/overload/thread_local_overload_state.h
@@ -28,7 +28,7 @@ class OverloadActionState {
 
   explicit constexpr OverloadActionState(UnitFloat value) : action_value_(value) {}
 
-  float value() const { return action_value_.value(); }
+  UnitFloat value() const { return action_value_; }
   bool isSaturated() const { return action_value_.value() == UnitFloat::max().value(); }
 
 private:

diff --git a/source/common/common/interval_value.h b/source/common/common/interval_value.h
@@ -22,6 +22,26 @@ template <typename T, typename Interval> class ClosedIntervalValue {
 
   T value() const { return value_; }
 
+  // Returns a value that is as far from max as the original value is from min.
+  // This guarantees that max().invert() == min() and min().invert() == max().
+  ClosedIntervalValue invert() const {
+    return ClosedIntervalValue(value_ == Interval::max_value
+                                   ? Interval::min_value
+                                   : value_ == Interval::min_value
+                                         ? Interval::max_value
+                                         : Interval::max_value - (value_ - Interval::min_value));
+  }
+
+  // Comparisons are performed using the same operators on the underlying value
+  // type, with the same exactness guarantees.
+
+  bool operator==(ClosedIntervalValue<T, Interval> other) const { return value_ == other.value(); }
+  bool operator!=(ClosedIntervalValue<T, Interval> other) const { return value_ != other.value(); }
+  bool operator<(ClosedIntervalValue<T, Interval> other) const { return value_ < other.value(); }
+  bool operator<=(ClosedIntervalValue<T, Interval> other) const { return value_ <= other.value(); }
+  bool operator>=(ClosedIntervalValue<T, Interval> other) const { return value_ >= other.value(); }
+  bool operator>(ClosedIntervalValue<T, Interval> other) const { return value_ > other.value(); }
+
 private:
   T value_;
 };

diff --git a/source/common/event/scaled_range_timer_manager_impl.cc b/source/common/event/scaled_range_timer_manager_impl.cc
@@ -150,9 +150,9 @@ TimerPtr ScaledRangeTimerManagerImpl::createTimer(ScaledTimerMinimum minimum, Ti
   return std::make_unique<RangeTimerImpl>(minimum, callback, *this);
 }
 
-void ScaledRangeTimerManagerImpl::setScaleFactor(double scale_factor) {
+void ScaledRangeTimerManagerImpl::setScaleFactor(UnitFloat scale_factor) {
   const MonotonicTime now = dispatcher_.approximateMonotonicTime();
-  scale_factor_ = UnitFloat(scale_factor);
+  scale_factor_ = scale_factor;
   for (auto& queue : queues_) {
     resetQueueTimer(*queue, now);
   }

diff --git a/source/common/event/scaled_range_timer_manager_impl.h b/source/common/event/scaled_range_timer_manager_impl.h
@@ -1,3 +1,5 @@
+#pragma once
+
 #include <chrono>
 #include <stack>
 
@@ -26,7 +28,7 @@ class ScaledRangeTimerManagerImpl : public ScaledRangeTimerManager {
 
   // ScaledRangeTimerManager impl
   TimerPtr createTimer(ScaledTimerMinimum minimum, TimerCb callback) override;
-  void setScaleFactor(double scale_factor) override;
+  void setScaleFactor(UnitFloat scale_factor) override;
 
 private:
   class RangeTimerImpl;

diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc
@@ -124,8 +124,7 @@ void TcpListenerImpl::enable() { socket_->ioHandle().enableFileEvents(Event::Fil
 
 void TcpListenerImpl::disable() { socket_->ioHandle().enableFileEvents(0); }
 
-void TcpListenerImpl::setRejectFraction(const float reject_fraction) {
-  ASSERT(0 <= reject_fraction && reject_fraction <= 1);
+void TcpListenerImpl::setRejectFraction(const UnitFloat reject_fraction) {
   reject_fraction_ = reject_fraction;
 }
 

diff --git a/source/common/network/tcp_listener_impl.h b/source/common/network/tcp_listener_impl.h
@@ -3,6 +3,8 @@
 #include "envoy/common/random_generator.h"
 #include "envoy/runtime/runtime.h"
 
+#include "common/common/interval_value.h"
+
 #include "absl/strings/string_view.h"
 #include "base_listener_impl.h"
 
@@ -20,7 +22,7 @@ class TcpListenerImpl : public BaseListenerImpl {
   ~TcpListenerImpl() override { socket_->ioHandle().resetFileEvents(); }
   void disable() override;
   void enable() override;
-  void setRejectFraction(float reject_fraction) override;
+  void setRejectFraction(UnitFloat reject_fraction) override;
 
   static const absl::string_view GlobalMaxCxRuntimeKey;
 
@@ -38,7 +40,7 @@ class TcpListenerImpl : public BaseListenerImpl {
   static bool rejectCxOverGlobalLimit();
 
   Random::RandomGenerator& random_;
-  float reject_fraction_;
+  UnitFloat reject_fraction_;
 };
 
 } // namespace Network

diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h
@@ -30,7 +30,7 @@ class UdpListenerImpl : public BaseListenerImpl,
   // Network::Listener Interface
   void disable() override;
   void enable() override;
-  void setRejectFraction(float) override {}
+  void setRejectFraction(UnitFloat) override {}
 
   // Network::UdpListener Interface
   Event::Dispatcher& dispatcher() override;

diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc
@@ -153,7 +153,7 @@ void ConnectionHandlerImpl::enableListeners() {
   }
 }
 
-void ConnectionHandlerImpl::setListenerRejectFraction(float reject_fraction) {
+void ConnectionHandlerImpl::setListenerRejectFraction(UnitFloat reject_fraction) {
   listener_reject_fraction_ = reject_fraction;
   for (auto& listener : listeners_) {
     listener.second.listener_->listener()->setRejectFraction(reject_fraction);

diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h
@@ -83,7 +83,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler,
   void stopListeners() override;
   void disableListeners() override;
   void enableListeners() override;
-  void setListenerRejectFraction(float reject_fraction) override;
+  void setListenerRejectFraction(UnitFloat reject_fraction) override;
   const std::string& statPrefix() const override { return per_handler_stat_prefix_; }
 
   /**
@@ -361,7 +361,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler,
   std::list<std::pair<Network::Address::InstanceConstSharedPtr, ActiveListenerDetails>> listeners_;
   std::atomic<uint64_t> num_handler_connections_{};
   bool disable_listeners_;
-  float listener_reject_fraction_{0};
+  UnitFloat listener_reject_fraction_{UnitFloat::min()};
 };
 
 class ActiveUdpListenerBase : public ConnectionHandlerImpl::ActiveListenerImplBase,

diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc
@@ -60,7 +60,11 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState {
   void setState(NamedOverloadActionSymbolTable::Symbol action, OverloadActionState state) {
     actions_[action.index()] = state;
     if (scaled_timer_action_.has_value() && scaled_timer_action_.value() == action) {
-      scaled_timer_manager_->setScaleFactor(1 - state.value());
+      scaled_timer_manager_->setScaleFactor(
+          // The action state is 0 for no overload up to 1 for maximal overload,
+          // but the scale factor for timers is 1 for no scaling and 0 for
+          // maximal scaling, so invert the value to pass in (1-value).
+          state.value().invert());
     }
   }
 
@@ -86,6 +90,8 @@ class ThresholdTriggerImpl final : public OverloadAction::Trigger {
     const OverloadActionState state = actionState();
     state_ =
         value >= threshold_ ? OverloadActionState::saturated() : OverloadActionState::inactive();
+    // This is a floating point comparison, though state_ is always either
+    // saturated or inactive so there's no risk due to floating point precision.
     return state.value() != actionState().value();
   }
 
@@ -117,6 +123,9 @@ class ScaledTriggerImpl final : public OverloadAction::Trigger {
       state_ = OverloadActionState(
           UnitFloat((value - scaling_threshold_) / (saturated_threshold_ - scaling_threshold_)));
     }
+    // All values of state_ are produced via this same code path. Even if
+    // old_state and state_ should be approximately equal, there's no harm in
+    // signaling for a small change if they're not float::operator== equal.
     return state_.value() != old_state.value();
   }
 
@@ -258,7 +267,7 @@ bool OverloadAction::updateResourcePressure(const std::string& name, double pres
   }
   const auto trigger_new_state = it->second->actionState();
   active_gauge_.set(trigger_new_state.isSaturated() ? 1 : 0);
-  scale_percent_gauge_.set(trigger_new_state.value() * 100);
+  scale_percent_gauge_.set(trigger_new_state.value().value() * 100);
 
   {
     // Compute the new state as the maximum over all trigger states.

diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc
@@ -152,7 +152,7 @@ void WorkerImpl::stopAcceptingConnectionsCb(OverloadActionState state) {
 }
 
 void WorkerImpl::rejectIncomingConnectionsCb(OverloadActionState state) {
-  handler_->setListenerRejectFraction(static_cast<float>(state.value()));
+  handler_->setListenerRejectFraction(state.value());
 }
 
 } // namespace Server

diff --git a/test/common/common/BUILD b/test/common/common/BUILD
@@ -196,6 +196,7 @@ envoy_cc_test(
     name = "random_generator_test",
     srcs = ["random_generator_test.cc"],
     deps = [
+        "//source/common/common:interval_value",
         "//source/common/common:random_generator_lib",
         "//test/mocks/runtime:runtime_mocks",
         "//test/test_common:environment_lib",

diff --git a/test/common/common/random_generator_test.cc b/test/common/common/random_generator_test.cc
@@ -1,5 +1,6 @@
 #include <numeric>
 
+#include "common/common/interval_value.h"
 #include "common/common/random_generator.h"
 
 #include "gmock/gmock.h"
@@ -70,15 +71,13 @@ TEST(UUID, SanityCheckOfUniqueness) {
 TEST(Random, Bernoilli) {
   Random::RandomGeneratorImpl random;
 
-  EXPECT_FALSE(random.bernoulli(0));
-  EXPECT_FALSE(random.bernoulli(-1));
-  EXPECT_TRUE(random.bernoulli(1));
-  EXPECT_TRUE(random.bernoulli(2));
+  EXPECT_FALSE(random.bernoulli(UnitFloat(0.0f)));
+  EXPECT_TRUE(random.bernoulli(UnitFloat(1.0f)));
 
   int true_count = 0;
   static const auto num_rolls = 100000;
   for (size_t i = 0; i < num_rolls; ++i) {
-    if (random.bernoulli(0.4)) {
+    if (random.bernoulli(UnitFloat(0.4f))) {
       ++true_count;
     }
   }