Skip to content
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

minor perf: update filter state method to reduce repeated hash searching #19609

Merged
merged 10 commits into from
Feb 10, 2022
Merged

minor perf: update filter state method to reduce repeated hash searching #19609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7adcdf3
update fromFilterState to reduce hash serching
wbpcode Jan 18, 2022
bc71b9d
minor update
wbpcode Jan 18, 2022
b8d02f3
minor update & fix format
wbpcode Jan 19, 2022
4addb66
code cleanup for filterstate
wbpcode Feb 8, 2022
53767aa
Merge branch 'main' of https://github.com/envoyproxy/envoy into filte…
wbpcode Feb 8, 2022
a901b02
fix some test
wbpcode Feb 8, 2022
9c62424
fix some test
wbpcode Feb 9, 2022
8df4b08
Merge branch 'main' of https://github.com/envoyproxy/envoy into filte…
wbpcode Feb 9, 2022
0e91746
update per file coverage to avoid ci flaky
wbpcode Feb 9, 2022
76fa3fd
minor updates to simplify code
wbpcode Feb 10, 2022
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
49 changes: 18 additions & 31 deletions envoy/stream_info/filter_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,52 +87,42 @@ class FilterState {

/**
* @param data_name the name of the data being looked up (mutable/readonly).
* @return a const reference to the stored data.
* An exception will be thrown if the data does not exist. This function
* will fail if the data stored under |data_name| cannot be dynamically
* cast to the type specified.
* @return a typed pointer to the stored data or nullptr if the data does not exist or the data
* type does not match the expected type.
*/
template <typename T> const T& getDataReadOnly(absl::string_view data_name) const {
const T* result = dynamic_cast<const T*>(getDataReadOnlyGeneric(data_name));
if (!result) {
ExceptionUtil::throwEnvoyException(
fmt::format("Data stored under {} cannot be coerced to specified type", data_name));
}
return *result;
template <typename T> const T* getDataReadOnly(absl::string_view data_name) const {
return dynamic_cast<const T*>(getDataReadOnlyGeneric(data_name));
}

/**
* @param data_name the name of the data being looked up (mutable/readonly).
* @return a const reference to the stored data.
* An exception will be thrown if the data does not exist.
* @return a const pointer to the stored data or nullptr if the data does not exist.
*/
virtual const Object* getDataReadOnlyGeneric(absl::string_view data_name) const PURE;

/**
* @param data_name the name of the data being looked up (mutable only).
* @return a non-const reference to the stored data if and only if the
* underlying data is mutable.
* An exception will be thrown if the data does not exist or if it is
* immutable. This function will fail if the data stored under
* |data_name| cannot be dynamically cast to the type specified.
* @param data_name the name of the data being looked up (mutable/readonly).
* @return a typed pointer to the stored data or nullptr if the data does not exist or the data
* type does not match the expected type.
*/
template <typename T> T& getDataMutable(absl::string_view data_name) {
T* result = dynamic_cast<T*>(getDataMutableGeneric(data_name));
if (!result) {
ExceptionUtil::throwEnvoyException(
fmt::format("Data stored under {} cannot be coerced to specified type", data_name));
}
return *result;
template <typename T> T* getDataMutable(absl::string_view data_name) {
return dynamic_cast<T*>(getDataMutableGeneric(data_name));
}

/**
* @param data_name the name of the data being looked up (mutable/readonly).
* @return a pointer to the stored data or nullptr if the data does not exist.
* An exception will be thrown if the data is not mutable.
*/
virtual Object* getDataMutableGeneric(absl::string_view data_name) PURE;

/**
* @param data_name the name of the data being probed.
* @return Whether data of the type and name specified exists in the
* data store.
*/
template <typename T> bool hasData(absl::string_view data_name) const {
Copy link
Member

Choose a reason for hiding this comment

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

Is this still used anywhere? Can it be removed to avoid using the pattern you removed in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is still used when we try to set some value to the filter state.

if(!filter_state.hasData<Type>(name)) {
  filter_state.setData(...);
}

Copy link
Member

Choose a reason for hiding this comment

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

Seems like this will also have the same double hash lookup issue? Maybe setData() should have a parameter about whether to set of it already exists? WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

get it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like it's more complicated than I thought it would be. The parent filter state and the mutable flag make it's hard to change the current hasData then setData code pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

@htuch Sounds reasonable to reserve two different API (getReadOnly/getMutable). But I am not sure if we should add special flag(StateType) for every filter state object.

Copy link
Member

Choose a reason for hiding this comment

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

I feel less strongly about the state type aspect. It seems like it could catch some kinds of errors, which all things being equal is a good thing. If there really is a genuine performance issue, we can probably eliminate, but I'd vote note to change it unless this is the cause of the observable performance loss.

Copy link
Member Author

Choose a reason for hiding this comment

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

@htuch StateType do not result in observable performance loss. But it makes the implementation of FilterState complex and there are some unsafe scenarios.

The following code is a common use of FilterState:

if(filter_state.hasData<Type>(name)) {
  auto state = filter_state.getDataMutable<Type>(...);
  // ......
}

But hasData will return true if there is a read only filter state object. Then the getDataMutable may throws un-catched exception.

However, the StateType can be used to avoid the FilterState Object being overwritten or modified. 🤔 So, there are both advantages and disadvantages.

Copy link
Member

Choose a reason for hiding this comment

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

The example you give is interesting. I think we should have a pattern such as getDataMutableOrNull and then use that instead of hasData; that seems the real unsafe thing. I'm not going to strongly advocate for this but I do see some advantage and fixing the above pattern in any case seems desirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds great. I will try to solve it in a new PR. 🤔

return (hasDataWithName(data_name) &&
(dynamic_cast<const T*>(getDataReadOnlyGeneric(data_name)) != nullptr));
return getDataReadOnly<T>(data_name) != nullptr;
}

/**
Expand All @@ -159,9 +149,6 @@ class FilterState {
* either the top LifeSpan or the parent is not yet created.
*/
virtual FilterStateSharedPtr parent() const PURE;

protected:
virtual Object* getDataMutableGeneric(absl::string_view data_name) PURE;
};

} // namespace StreamInfo
Expand Down
5 changes: 1 addition & 4 deletions source/common/formatter/substitution_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,10 +1490,7 @@ FilterStateFormatter::FilterStateFormatter(const std::string& key,
const Envoy::StreamInfo::FilterState::Object*
FilterStateFormatter::filterState(const StreamInfo::StreamInfo& stream_info) const {
const StreamInfo::FilterState& filter_state = stream_info.filterState();
if (!filter_state.hasDataWithName(key_)) {
return nullptr;
}
return &filter_state.getDataReadOnly<StreamInfo::FilterState::Object>(key_);
return filter_state.getDataReadOnly<StreamInfo::FilterState::Object>(key_);
}

absl::optional<std::string> FilterStateFormatter::format(const Http::RequestHeaderMap&,
Expand Down
4 changes: 2 additions & 2 deletions source/common/http/hash_policy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,8 @@ class FilterStateHashMethod : public HashMethodImplBase {
evaluate(const Network::Address::Instance*, const RequestHeaderMap&,
const HashPolicy::AddCookieCallback,
const StreamInfo::FilterStateSharedPtr filter_state) const override {
if (filter_state->hasData<Hashable>(key_)) {
return filter_state->getDataReadOnly<Hashable>(key_).hash();
if (auto typed_state = filter_state->getDataReadOnly<Hashable>(key_); typed_state != nullptr) {
return typed_state->hash();
}
return absl::nullopt;
}
Expand Down
4 changes: 2 additions & 2 deletions source/common/network/hash_policy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ class FilterStateHashMethod : public HashPolicyImpl::HashMethod {

absl::optional<uint64_t> evaluate(const Network::Connection& connection) const override {
const auto& filter_state = connection.streamInfo().filterState();
if (filter_state.hasData<Hashable>(key_)) {
return filter_state.getDataReadOnly<Hashable>(key_).hash();
if (auto typed_state = filter_state.getDataReadOnly<Hashable>(key_); typed_state != nullptr) {
return typed_state->hash();
}
return absl::nullopt;
}
Expand Down
31 changes: 15 additions & 16 deletions source/common/network/transport_socket_options_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,31 +67,30 @@ TransportSocketOptionsUtility::fromFilterState(const StreamInfo::FilterState& fi
absl::optional<Network::ProxyProtocolData> proxy_protocol_options;

bool needs_transport_socket_options = false;
if (filter_state.hasData<UpstreamServerName>(UpstreamServerName::key())) {
const auto& upstream_server_name =
filter_state.getDataReadOnly<UpstreamServerName>(UpstreamServerName::key());
server_name = upstream_server_name.value();
if (auto typed_data = filter_state.getDataReadOnly<UpstreamServerName>(UpstreamServerName::key());
typed_data != nullptr) {
server_name = typed_data->value();
needs_transport_socket_options = true;
}

if (filter_state.hasData<Network::ApplicationProtocols>(Network::ApplicationProtocols::key())) {
const auto& alpn = filter_state.getDataReadOnly<Network::ApplicationProtocols>(
Network::ApplicationProtocols::key());
application_protocols = alpn.value();
if (auto typed_data = filter_state.getDataReadOnly<Network::ApplicationProtocols>(
Network::ApplicationProtocols::key());
typed_data != nullptr) {
application_protocols = typed_data->value();
needs_transport_socket_options = true;
}

if (filter_state.hasData<UpstreamSubjectAltNames>(UpstreamSubjectAltNames::key())) {
const auto& upstream_subject_alt_names =
filter_state.getDataReadOnly<UpstreamSubjectAltNames>(UpstreamSubjectAltNames::key());
subject_alt_names = upstream_subject_alt_names.value();
if (auto typed_data =
filter_state.getDataReadOnly<UpstreamSubjectAltNames>(UpstreamSubjectAltNames::key());
typed_data != nullptr) {
subject_alt_names = typed_data->value();
needs_transport_socket_options = true;
}

if (filter_state.hasData<ProxyProtocolFilterState>(ProxyProtocolFilterState::key())) {
const auto& proxy_protocol_filter_state =
filter_state.getDataReadOnly<ProxyProtocolFilterState>(ProxyProtocolFilterState::key());
proxy_protocol_options.emplace(proxy_protocol_filter_state.value());
if (auto typed_data =
filter_state.getDataReadOnly<ProxyProtocolFilterState>(ProxyProtocolFilterState::key());
typed_data != nullptr) {
proxy_protocol_options.emplace(typed_data->value());
needs_transport_socket_options = true;
}

Expand Down
11 changes: 6 additions & 5 deletions source/common/router/config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,11 +676,12 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::RequestHeaderMap& headers,
// Restore the port if this was a CONNECT request.
// Note this will restore the port for HTTP/2 CONNECT-upgrades as well as as HTTP/1.1 style
// CONNECT requests.
if (stream_info.filterState().hasData<OriginalConnectPort>(OriginalConnectPort::key()) &&
Http::HeaderUtility::getPortStart(headers.getHostValue()) == absl::string_view::npos) {
const OriginalConnectPort& original_port =
stream_info.filterState().getDataReadOnly<OriginalConnectPort>(OriginalConnectPort::key());
headers.setHost(absl::StrCat(headers.getHostValue(), ":", original_port.value()));
if (Http::HeaderUtility::getPortStart(headers.getHostValue()) == absl::string_view::npos) {
if (auto typed_state = stream_info.filterState().getDataReadOnly<OriginalConnectPort>(
OriginalConnectPort::key());
typed_state != nullptr) {
headers.setHost(absl::StrCat(headers.getHostValue(), ":", typed_state->value()));
}
}

if (!host_rewrite_.empty()) {
Expand Down
9 changes: 3 additions & 6 deletions source/common/router/header_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,21 +179,18 @@ parsePerRequestStateField(absl::string_view param_str) {
return [param](const Envoy::StreamInfo::StreamInfo& stream_info) -> std::string {
const Envoy::StreamInfo::FilterState& filter_state = stream_info.filterState();

// No such value means don't output anything.
if (!filter_state.hasDataWithName(param)) {
return std::string();
}
auto typed_state = filter_state.getDataReadOnly<StringAccessor>(param);

// Value exists but isn't string accessible is a contract violation; throw an error.
if (!filter_state.hasData<StringAccessor>(param)) {
if (typed_state == nullptr) {
ENVOY_LOG_MISC(debug,
"Invalid header information: PER_REQUEST_STATE value \"{}\" "
"exists but is not string accessible",
param);
return std::string();
}

return std::string(filter_state.getDataReadOnly<StringAccessor>(param).asString());
return std::string(typed_state->asString());
};
}

Expand Down
54 changes: 25 additions & 29 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,10 +385,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
// we should append cluster and host headers to the response, and whether to forward the request
// upstream.
const StreamInfo::FilterStateSharedPtr& filter_state = callbacks_->streamInfo().filterState();
const DebugConfig* debug_config =
filter_state->hasData<DebugConfig>(DebugConfig::key())
? &(filter_state->getDataReadOnly<DebugConfig>(DebugConfig::key()))
: nullptr;
const DebugConfig* debug_config = filter_state->getDataReadOnly<DebugConfig>(DebugConfig::key());

// TODO: Maybe add a filter API for this.
grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers);
Expand Down Expand Up @@ -567,24 +564,18 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
transport_socket_options_ = Network::TransportSocketOptionsUtility::fromFilterState(
*callbacks_->streamInfo().filterState());

auto has_options_from_downstream =
downstreamConnection() && downstreamConnection()
->streamInfo()
.filterState()
.hasData<Network::UpstreamSocketOptionsFilterState>(
Network::UpstreamSocketOptionsFilterState::key());

if (has_options_from_downstream) {
auto downstream_options = downstreamConnection()
->streamInfo()
.filterState()
.getDataReadOnly<Network::UpstreamSocketOptionsFilterState>(
Network::UpstreamSocketOptionsFilterState::key())
.value();
if (!upstream_options_) {
upstream_options_ = std::make_shared<Network::Socket::Options>();
if (auto downstream_connection = downstreamConnection(); downstream_connection != nullptr) {
if (auto typed_state = downstream_connection->streamInfo()
.filterState()
.getDataReadOnly<Network::UpstreamSocketOptionsFilterState>(
Network::UpstreamSocketOptionsFilterState::key());
typed_state != nullptr) {
auto downstream_options = typed_state->value();
if (!upstream_options_) {
upstream_options_ = std::make_shared<Network::Socket::Options>();
}
Network::Socket::appendOptions(upstream_options_, downstream_options);
}
Network::Socket::appendOptions(upstream_options_, downstream_options);
}

if (upstream_options_ && callbacks_->getUpstreamSocketOptions()) {
Expand Down Expand Up @@ -1615,15 +1606,20 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do
const StreamInfo::FilterStateSharedPtr& filter_state = callbacks_->streamInfo().filterState();
// Make sure that performing the redirect won't result in exceeding the configured number of
// redirects allowed for this route.
if (!filter_state->hasData<StreamInfo::UInt32Accessor>(NumInternalRedirectsFilterStateName)) {
filter_state->setData(
NumInternalRedirectsFilterStateName, std::make_shared<StreamInfo::UInt32AccessorImpl>(0),
StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Request);
StreamInfo::UInt32Accessor* num_internal_redirect{};

if (num_internal_redirect = filter_state->getDataMutable<StreamInfo::UInt32Accessor>(
NumInternalRedirectsFilterStateName);
num_internal_redirect == nullptr) {
auto state = std::make_shared<StreamInfo::UInt32AccessorImpl>(0);
num_internal_redirect = state.get();

filter_state->setData(NumInternalRedirectsFilterStateName, std::move(state),
StreamInfo::FilterState::StateType::Mutable,
StreamInfo::FilterState::LifeSpan::Request);
}
StreamInfo::UInt32Accessor& num_internal_redirect =
filter_state->getDataMutable<StreamInfo::UInt32Accessor>(NumInternalRedirectsFilterStateName);

if (num_internal_redirect.value() >= policy.maxInternalRedirects()) {
if (num_internal_redirect->value() >= policy.maxInternalRedirects()) {
ENVOY_STREAM_LOG(trace, "Internal redirect failed: redirect limits exceeded.", *callbacks_);
config_.stats_.passthrough_internal_redirect_too_many_redirects_.inc();
return false;
Expand Down Expand Up @@ -1687,7 +1683,7 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do
callbacks_->modifyDecodingBuffer([](Buffer::Instance& data) { data.drain(data.length()); });
}

num_internal_redirect.increment();
num_internal_redirect->increment();
restore_original_headers.cancel();
// Preserve the original request URL for the second pass.
downstream_headers.setEnvoyOriginalUrl(absl::StrCat(scheme_is_http
Expand Down
10 changes: 5 additions & 5 deletions source/common/stream_info/filter_state_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ bool FilterStateImpl::hasDataWithName(absl::string_view data_name) const {

const FilterState::Object*
FilterStateImpl::getDataReadOnlyGeneric(absl::string_view data_name) const {
const auto& it = data_storage_.find(data_name);
const auto it = data_storage_.find(data_name);

if (it == data_storage_.end()) {
if (parent_) {
return &(parent_->getDataReadOnly<FilterState::Object>(data_name));
return parent_->getDataReadOnlyGeneric(data_name);
}
throw EnvoyException("FilterState::getDataReadOnly<T> called for unknown data name.");
return nullptr;
}

const FilterStateImpl::FilterObject* current = it->second.get();
Expand All @@ -65,9 +65,9 @@ FilterState::Object* FilterStateImpl::getDataMutableGeneric(absl::string_view da

if (it == data_storage_.end()) {
if (parent_) {
return &(parent_->getDataMutable<FilterState::Object>(data_name));
return parent_->getDataMutableGeneric(data_name);
}
throw EnvoyException("FilterState::getDataMutable<T> called for unknown data name.");
return nullptr;
}

FilterStateImpl::FilterObject* current = it->second.get();
Expand Down
47 changes: 20 additions & 27 deletions source/common/tcp_proxy/tcp_proxy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,11 @@ Config::Config(const envoy::extensions::filters::network::tcp_proxy::v3::TcpProx

RouteConstSharedPtr Config::getRegularRouteFromEntries(Network::Connection& connection) {
// First check if the per-connection state to see if we need to route to a pre-selected cluster
if (connection.streamInfo().filterState()->hasData<PerConnectionCluster>(
PerConnectionCluster::key())) {
const PerConnectionCluster& per_connection_cluster =
connection.streamInfo().filterState()->getDataReadOnly<PerConnectionCluster>(
PerConnectionCluster::key());

return std::make_shared<const SimpleRouteImpl>(*this, per_connection_cluster.value());
if (const auto* per_connection_cluster =
connection.streamInfo().filterState()->getDataReadOnly<PerConnectionCluster>(
PerConnectionCluster::key());
per_connection_cluster != nullptr) {
return std::make_shared<const SimpleRouteImpl>(*this, per_connection_cluster->value());
}

if (default_route_ != nullptr) {
Expand Down Expand Up @@ -363,37 +361,32 @@ Network::FilterStatus Filter::initializeUpstreamConnection() {
return Network::FilterStatus::StopIteration;
}

if (downstreamConnection()) {
if (auto downstream_connection = downstreamConnection(); downstream_connection != nullptr) {
if (!read_callbacks_->connection()
.streamInfo()
.filterState()
->hasData<Network::ProxyProtocolFilterState>(
Network::ProxyProtocolFilterState::key())) {
read_callbacks_->connection().streamInfo().filterState()->setData(
Network::ProxyProtocolFilterState::key(),
std::make_unique<Network::ProxyProtocolFilterState>(Network::ProxyProtocolData{
downstreamConnection()->connectionInfoProvider().remoteAddress(),
downstreamConnection()->connectionInfoProvider().localAddress()}),
std::make_shared<Network::ProxyProtocolFilterState>(Network::ProxyProtocolData{
downstream_connection->connectionInfoProvider().remoteAddress(),
downstream_connection->connectionInfoProvider().localAddress()}),
StreamInfo::FilterState::StateType::ReadOnly,
StreamInfo::FilterState::LifeSpan::Connection);
}
transport_socket_options_ = Network::TransportSocketOptionsUtility::fromFilterState(
downstreamConnection()->streamInfo().filterState());

auto has_options_from_downstream =
downstreamConnection() && downstreamConnection()
->streamInfo()
.filterState()
.hasData<Network::UpstreamSocketOptionsFilterState>(
Network::UpstreamSocketOptionsFilterState::key());
if (has_options_from_downstream) {
auto downstream_options = downstreamConnection()
->streamInfo()
.filterState()
.getDataReadOnly<Network::UpstreamSocketOptionsFilterState>(
Network::UpstreamSocketOptionsFilterState::key())
.value();
upstream_options_ = std::make_shared<Network::Socket::Options>();
downstream_connection->streamInfo().filterState());

if (auto typed_state = downstream_connection->streamInfo()
.filterState()
.getDataReadOnly<Network::UpstreamSocketOptionsFilterState>(
Network::UpstreamSocketOptionsFilterState::key());
typed_state != nullptr) {
auto downstream_options = typed_state->value();
if (!upstream_options_) {
upstream_options_ = std::make_shared<Network::Socket::Options>();
}
Network::Socket::appendOptions(upstream_options_, downstream_options);
}
}
Expand Down
Loading