Skip to content

refactor: use unitfloat in more places #14396

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.

Sign up for GitHub

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

Merged
jmarantz merged 10 commits into from
Jan 13, 2021
Merged

refactor: use unitfloat in more places #14396

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b46c1e9
Use UnitFloat for setScaleFactor
akonradi Dec 11, 2020
d68720c
Use UnitFloat in more places
akonradi Dec 14, 2020
d6fccf7
Fix random generator test
akonradi Dec 14, 2020
a39e5f9
Remove UnitFloat::operator== and !=
akonradi Jan 11, 2021
2388799
Revert "Remove UnitFloat::operator== and !="
akonradi Jan 12, 2021
5e6758a
Add comments about floating point comparisons
akonradi Jan 12, 2021
d06309e
Add a ClosedIntervalValue::invert()
akonradi Jan 12, 2021
70295b3
Add comment about invert()
akonradi Jan 13, 2021
933a157
Fix formatting
akonradi Jan 13, 2021
856a63c
Fix spelling
akonradi Jan 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/envoy/common/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
10 changes: 6 additions & 4 deletions include/envoy/common/random_generator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#include "envoy/common/pure.h"

#include "common/common/interval_value.h"

namespace Envoy {
namespace Random {

Expand Down Expand Up @@ -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.isMin()) {
return false;
} else if (p >= 1) {
} else if (p.isMax()) {
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()));
}
};

Expand Down
4 changes: 3 additions & 1 deletion include/envoy/event/scaled_range_timer_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;

@KBaichoo KBaichoo Dec 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are there any usages of this where the precision downgrade will make a difference?

@akonradi akonradi Dec 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, this might make a difference of a few milliseconds on a 10 minute timer but that's not worth worrying about.

virtual void setScaleFactor(UnitFloat scale_factor) PURE;
};

using ScaledRangeTimerManagerPtr = std::unique_ptr<ScaledRangeTimerManager>;
Expand Down
2 changes: 2 additions & 0 deletions include/envoy/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ envoy_cc_library(
":listen_socket_interface",
":listener_interface",
"//include/envoy/ssl:context_interface",
"//source/common/common:interval_value",
],
)

Expand Down Expand Up @@ -172,6 +173,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",
],
)
Expand Down
4 changes: 3 additions & 1 deletion include/envoy/network/connection_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "envoy/network/listener.h"
#include "envoy/ssl/context.h"

#include "common/common/interval_value.h"

namespace Envoy {
namespace Network {

Expand Down Expand Up @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion include/envoy/network/listener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -333,7 +335,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>;
Expand Down
2 changes: 1 addition & 1 deletion include/envoy/server/overload/thread_local_overload_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
8 changes: 8 additions & 0 deletions source/common/common/interval_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ template <typename T, typename Interval> class ClosedIntervalValue {

T value() const { return value_; }

bool isMin() const { return value_ == Interval::min_value; }
bool isMax() const { return value_ == Interval::max_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_;
};
Expand Down
4 changes: 2 additions & 2 deletions source/common/event/scaled_range_timer_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
4 changes: 3 additions & 1 deletion source/common/event/scaled_range_timer_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#pragma once

#include <chrono>
#include <stack>

Expand Down Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions source/common/network/tcp_listener_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
6 changes: 4 additions & 2 deletions source/common/network/tcp_listener_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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;

Expand All @@ -38,7 +40,7 @@ class TcpListenerImpl : public BaseListenerImpl {
static bool rejectCxOverGlobalLimit();

Random::RandomGenerator& random_;
float reject_fraction_;
UnitFloat reject_fraction_;
};

} // namespace Network
Expand Down
2 changes: 1 addition & 1 deletion source/common/network/udp_listener_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion source/server/connection_handler_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions source/server/connection_handler_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_; }

/**
Expand Down Expand Up @@ -363,7 +363,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,
Expand Down
10 changes: 5 additions & 5 deletions source/server/overload_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ 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(UnitFloat(1 - state.value().value()));
}
}

Expand All @@ -86,7 +86,7 @@ class ThresholdTriggerImpl final : public OverloadAction::Trigger {
const OverloadActionState state = actionState();
state_ =
value >= threshold_ ? OverloadActionState::saturated() : OverloadActionState::inactive();
return state.value() != actionState().value();
return state.value().value() != actionState().value().value();

@jmarantz jmarantz Jan 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you are going to fall back to this, I don't think we've actually improved anything, right?

2 other options I've thought of:

  • provide operator== but make its implementation approximate, e.g. abs(a - b) < K*(max-min)
  • provide methods that invert a value between min and max without losing exactness

@akonradi akonradi Jan 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This makes the floating point equality comparison visible instead of hiding it behind UnitFloat::operator==. I could see the argument for comparing with a fudge factor but my preference would be to do that here since it's a statement about how precise the OverloadManagerImpl wants to be, not about UnitFloat values.

@jmarantz jmarantz Jan 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this file, on line 63, you set the value to 1-value.

If this is not exact, then we have a potential problem, right?

@jmarantz jmarantz Jan 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And if you still don't like the approximate comparison (which I can understand) it looks like it would be easy enough for you to replace the 1-value pattern with an exactness-preserving inversion operation.

I think if you just pull the floats out and compare them then this is a bit of a leaky abstraction, right?

@akonradi akonradi Jan 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see the 1-value on line 63, but I'm having trouble understanding the potential problem. Are you worried that UnitFloat(1 - UnitFloat::max()) is not equal to UnitFloat::min() and vice-versa?

this is a bit of a leaky abstraction, right?

UnitFloat only guarantees that if you call .value(), the value you get out is in the range [0, 1], and that it is as close as possible to the float provided in the constructor.

@akonradi akonradi Jan 12, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Talked offline with @jmarantz; added comments and UnitFloat::invert() for the 1-pattern, and added back in UnitFloat::operator== and !=

}

OverloadActionState actionState() const override { return state_; }
Expand Down Expand Up @@ -117,7 +117,7 @@ class ScaledTriggerImpl final : public OverloadAction::Trigger {
state_ = OverloadActionState(
UnitFloat((value - scaling_threshold_) / (saturated_threshold_ - scaling_threshold_)));
}
return state_.value() != old_state.value();
return state_.value().value() != old_state.value().value();
}

OverloadActionState actionState() const override { return state_; }
Expand Down Expand Up @@ -258,7 +258,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.
Expand All @@ -272,7 +272,7 @@ bool OverloadAction::updateResourcePressure(const std::string& name, double pres
state_ = new_state;
}

return state_.value() != old_state.value();
return state_.value().value() != old_state.value().value();
}

OverloadActionState OverloadAction::getState() const { return state_; }
Expand Down
2 changes: 1 addition & 1 deletion source/server/worker_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions test/common/common/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
9 changes: 4 additions & 5 deletions test/common/common/random_generator_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <numeric>

#include "common/common/interval_value.h"
#include "common/common/random_generator.h"

#include "gmock/gmock.h"
Expand Down Expand Up @@ -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;
}
}
Expand Down
Loading