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

Jump to bottom
Merged
merged 2 commits into from
Jul 12, 2019
Merged

filterchain: use span to avoid construct vector #7523

merged 2 commits into from
Jul 12, 2019

Conversation

lambdai
Copy link
Contributor

@lambdai lambdai commented Jul 10, 2019 •
edited
Loading

Signed-off-by: Yuchen Dai silentdai@gmail.com

Description:
Clean up the TODO
The read-only Protobuf::RepeatedField can be viewed as span in listener/filterchain

Risk Level: LOW. It's internal implementation detail of filter chain.
Testing:
Existing test.
Filterchain benchmark test doesn't complain in trivial case. But I didn't extend the test to embrace huge server names and protocol set.

lambdai added 2 commits July 10, 2019 17:47
@lambdai
filterchain: use span
245851e 
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@lambdai
remove TODO line
5356c22 
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@lambdai lambdai marked this pull request as ready for review July 10, 2019 19:59
dnoe
dnoe reviewed Jul 11, 2019
View reviewed changes
source/server/filter_chain_manager_impl.cc
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

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

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

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

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

@lambdai
Copy link
Contributor Author

lambdai commented Jul 12, 2019

Sorry Combinations of Span< [const, nonconst] std::reference_wrapper<[const, nonconst] std::string> > is not convertible from RepeatedPtrField. I prefer land this PR first and explore in the future.

class RefWrapperTest : public testing::Test {
public:
  void takeWrapper(absl::Span<std::reference_wrapper<std::string>> foo) { 
    UNREFERENCED_PARAMETER(foo);
  }
};
TEST_F(RefWrapperTest, VectorToRefSpan) {
  envoy::api::v2::listener::FilterChainMatch m;
  takeWrapper(m.server_names());
}

test/server/filter_chain_manager_impl_test.cc:157:15: error: no viable conversion from 'const ::google::protobuf::RepeatedPtrField< ::std::string>' (aka 'const RepeatedPtrField<basic_string<char> >') to 'absl::Span<const std::reference_wrapper<const std
::string> >' (aka 'Span<const reference_wrapper<const basic_string<char> > >')

test/server/filter_chain_manager_impl_test.cc:157:15: error: no viable conversion from 'const ::google::protobuf::RepeatedPtrField< ::std::string>' (aka 'const RepeatedPtrField<basic_string<char> >') to 'absl::Span<const std::reference_wrapper<std::stri
ng> >' (aka 'Span<const reference_wrapper<basic_string<char> > >')

test/server/filter_chain_manager_impl_test.cc:157:15: error: no viable conversion from 'const ::google::protobuf::RepeatedPtrField< ::std::string>' (aka 'const RepeatedPtrField<basic_string<char> >') to 'absl::Span<std::reference_wrapper<const std::stri
ng> >' (aka 'Span<reference_wrapper<const basic_string<char> > >')

test/server/filter_chain_manager_impl_test.cc:157:15: error: no viable conversion from 'const ::google::protobuf::RepeatedPtrField< ::std::string>' (aka 'const RepeatedPtrField<basic_string<char> >') to 'absl::Span<std::reference_wrapper<std::string> >'
 (aka 'Span<reference_wrapper<basic_string<char> > >')

dnoe
dnoe approved these changes Jul 12, 2019
View reviewed changes
Copy link
Contributor

@dnoe dnoe left a comment

Choose a reason for hiding this comment

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

SGTM, thanks for looking into it.

mattklein123
mattklein123 approved these changes Jul 12, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit e591a13 into envoyproxy:master Jul 12, 2019
@lambdai lambdai deleted the td_span branch July 15, 2019 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+1 more reviewer

@dnoe dnoe dnoe approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@dnoe dnoe

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lambdai @dnoe @mattklein123