Skip to content

filterchain: use span to avoid construct vector #7523

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
mattklein123 merged 2 commits into from
Jul 12, 2019
Merged

filterchain: use span to avoid construct vector #7523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
245851e
filterchain: use span
lambdai Jul 9, 2019
5356c22
remove TODO line
lambdai Jul 10, 2019
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
59 changes: 27 additions & 32 deletions source/server/filter_chain_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,19 +72,12 @@ void FilterChainManagerImpl::addFilterChain(
}
}

std::vector<std::string> server_names(filter_chain_match.server_names().begin(),
filter_chain_match.server_names().end());

std::vector<std::string> application_protocols(
filter_chain_match.application_protocols().begin(),
filter_chain_match.application_protocols().end());

// TODO(silentdai): use absl::Span to avoid vector construction at server_names and alpn
addFilterChainForDestinationPorts(
destination_ports_map_,
PROTOBUF_GET_WRAPPED_OR_DEFAULT(filter_chain_match, destination_port, 0), destination_ips,
server_names, filter_chain_match.transport_protocol(), application_protocols,
filter_chain_match.source_type(), source_ips, filter_chain_match.source_ports(),
filter_chain_match.server_names(), filter_chain_match.transport_protocol(),
filter_chain_match.application_protocols(), filter_chain_match.source_type(), source_ips,
filter_chain_match.source_ports(),
std::shared_ptr<Network::FilterChain>(
filter_chain_factory_builder.buildFilterChain(*filter_chain)));
}
Expand All @@ -93,11 +86,12 @@ void FilterChainManagerImpl::addFilterChain(

void FilterChainManagerImpl::addFilterChainForDestinationPorts(
DestinationPortsMap& destination_ports_map, uint16_t destination_port,
const std::vector<std::string>& destination_ips, const std::vector<std::string>& server_names,
const std::string& transport_protocol, const std::vector<std::string>& application_protocols,
const std::vector<std::string>& destination_ips,
const absl::Span<const std::string* const> server_names, const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
Copy link
Contributor

@dnoe dnoe Jul 11, 2019

Choose a reason for hiding this comment

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

Is there a particular reason why you've chosen to use std::string* over something like std::reference_wrapper? I assume it's not possible to put a const reference in the Span?

Copy link
Contributor Author

@lambdai lambdai Jul 12, 2019

Choose a reason for hiding this comment

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

The only reason is that const absl::Span<const std::string* const> can be converted from const ::google::protobuf::RepeatedPtrField<::std::string> at no cost. Let me see if span<std::reference_wrapper> works
Below is the signature of protobuf generated function signature.

const ::google::protobuf::RepeatedPtrField<::std::string>&
FilterChainMatch::application_protocols() const

Copy link
Contributor

@dnoe dnoe Jul 12, 2019

Choose a reason for hiding this comment

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

A reference wrapper is basically a very thinly wrapped pointer, with the logic required to copy it (the pointer, not the object). It should be basically equivalent to the pointer case in terms of actual generated code. If it doesn't work right no worries, but there is some small benefit of reference_wrapper in conveying that the reference cannot be nullable and is not owned.

Copy link
Contributor Author

@lambdai lambdai Jul 12, 2019

Choose a reason for hiding this comment

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

Thanks! I am writing a small test cases to see if protobuf::RepeatedPtrField converts to Span<reference_wrapper>. Will post the code here. As long as it works I agree reference_wrapper is the preference

const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain) {
if (destination_ports_map.find(destination_port) == destination_ports_map.end()) {
destination_ports_map[destination_port] =
Expand All @@ -110,11 +104,11 @@ void FilterChainManagerImpl::addFilterChainForDestinationPorts(

void FilterChainManagerImpl::addFilterChainForDestinationIPs(
DestinationIPsMap& destination_ips_map, const std::vector<std::string>& destination_ips,
const std::vector<std::string>& server_names, const std::string& transport_protocol,
const std::vector<std::string>& application_protocols,
const absl::Span<const std::string* const> server_names, const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain) {
if (destination_ips.empty()) {
addFilterChainForServerNames(destination_ips_map[EMPTY_STRING], server_names,
Expand All @@ -130,11 +124,12 @@ void FilterChainManagerImpl::addFilterChainForDestinationIPs(
}

void FilterChainManagerImpl::addFilterChainForServerNames(
ServerNamesMapSharedPtr& server_names_map_ptr, const std::vector<std::string>& server_names,
const std::string& transport_protocol, const std::vector<std::string>& application_protocols,
ServerNamesMapSharedPtr& server_names_map_ptr,
const absl::Span<const std::string* const> server_names, const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain) {
if (server_names_map_ptr == nullptr) {
server_names_map_ptr = std::make_shared<ServerNamesMap>();
Expand All @@ -146,35 +141,35 @@ void FilterChainManagerImpl::addFilterChainForServerNames(
application_protocols, source_type, source_ips,
source_ports, filter_chain);
} else {
for (const auto& server_name : server_names) {
if (isWildcardServerName(server_name)) {
for (const auto& server_name_ptr : server_names) {
if (isWildcardServerName(*server_name_ptr)) {
// Add mapping for the wildcard domain, i.e. ".example.com" for "*.example.com".
addFilterChainForApplicationProtocols(
server_names_map[server_name.substr(1)][transport_protocol], application_protocols,
server_names_map[server_name_ptr->substr(1)][transport_protocol], application_protocols,
source_type, source_ips, source_ports, filter_chain);
} else {
addFilterChainForApplicationProtocols(server_names_map[server_name][transport_protocol],
application_protocols, source_type, source_ips,
source_ports, filter_chain);
addFilterChainForApplicationProtocols(
server_names_map[*server_name_ptr][transport_protocol], application_protocols,
source_type, source_ips, source_ports, filter_chain);
}
}
}
}

void FilterChainManagerImpl::addFilterChainForApplicationProtocols(
ApplicationProtocolsMap& application_protocols_map,
const std::vector<std::string>& application_protocols,
const absl::Span<const std::string* const> application_protocols,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain) {
if (application_protocols.empty()) {
addFilterChainForSourceTypes(application_protocols_map[EMPTY_STRING], source_type, source_ips,
source_ports, filter_chain);
} else {
for (const auto& application_protocol : application_protocols) {
addFilterChainForSourceTypes(application_protocols_map[application_protocol], source_type,
source_ips, source_ports, filter_chain);
for (const auto& application_protocol_ptr : application_protocols) {
addFilterChainForSourceTypes(application_protocols_map[*application_protocol_ptr],
source_type, source_ips, source_ports, filter_chain);
}
}
}
Expand All @@ -183,7 +178,7 @@ void FilterChainManagerImpl::addFilterChainForSourceTypes(
SourceTypesArray& source_types_array,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain) {
if (source_ips.empty()) {
addFilterChainForSourceIPs(source_types_array[source_type].first, EMPTY_STRING, source_ports,
Expand All @@ -198,7 +193,7 @@ void FilterChainManagerImpl::addFilterChainForSourceTypes(

void FilterChainManagerImpl::addFilterChainForSourceIPs(
SourceIPsMap& source_ips_map, const std::string& source_ip,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain) {
if (source_ports.empty()) {
addFilterChainForSourcePorts(source_ips_map[source_ip], 0, filter_chain);
Expand Down
31 changes: 18 additions & 13 deletions source/server/filter_chain_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,42 +62,47 @@ class FilterChainManagerImpl : public Network::FilterChainManager,

void addFilterChainForDestinationPorts(
DestinationPortsMap& destination_ports_map, uint16_t destination_port,
const std::vector<std::string>& destination_ips, const std::vector<std::string>& server_names,
const std::string& transport_protocol, const std::vector<std::string>& application_protocols,
const std::vector<std::string>& destination_ips,
const absl::Span<const std::string* const> server_names,
const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain);
void addFilterChainForDestinationIPs(
DestinationIPsMap& destination_ips_map, const std::vector<std::string>& destination_ips,
const std::vector<std::string>& server_names, const std::string& transport_protocol,
const std::vector<std::string>& application_protocols,
const absl::Span<const std::string* const> server_names,
const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain);
void addFilterChainForServerNames(
ServerNamesMapSharedPtr& server_names_map_ptr, const std::vector<std::string>& server_names,
const std::string& transport_protocol, const std::vector<std::string>& application_protocols,
ServerNamesMapSharedPtr& server_names_map_ptr,
const absl::Span<const std::string* const> server_names,
const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain);
void addFilterChainForApplicationProtocols(
ApplicationProtocolsMap& application_protocol_map,
const std::vector<std::string>& application_protocols,
const absl::Span<const std::string* const> application_protocols,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain);
void addFilterChainForSourceTypes(
SourceTypesArray& source_types_array,
const envoy::api::v2::listener::FilterChainMatch_ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain);
void addFilterChainForSourceIPs(SourceIPsMap& source_ips_map, const std::string& source_ip,
const Protobuf::RepeatedField<Protobuf::uint32>& source_ports,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain);
void addFilterChainForSourcePorts(SourcePortsMapSharedPtr& source_ports_map_ptr,
uint32_t source_port,
Expand Down