Skip to content

header_map: copy constructor for HeaderMapImpl. #4129

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
htuch merged 11 commits into from
Aug 14, 2018
Merged

header_map: copy constructor for HeaderMapImpl. #4129

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
01f7423
header_map: copy constructor for HeaderMapImpl.
htuch Aug 13, 2018
fffdbbc
Mark HeaderList non-copyable, explicit copy assignment operator for T…
htuch Aug 13, 2018
b0aa98a
Kick CI
htuch Aug 13, 2018
e5ce010
Kick CI
htuch Aug 13, 2018
76dfa6c
Add explicit in various places.
htuch Aug 13, 2018
080a830
fix_format
htuch Aug 13, 2018
f88da73
Finally it clicks, we need to prevent implicit move constructor.
htuch Aug 13, 2018
86cc2fa
Better explanation as per @jsedgwick.
htuch Aug 14, 2018
d7d65ad
wip
htuch Aug 14, 2018
3c525eb
More review feedback.
htuch Aug 14, 2018
aa008ff
Comment.
htuch Aug 14, 2018
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
33 changes: 19 additions & 14 deletions source/common/http/header_map_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,21 +276,10 @@ void HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data)

HeaderMapImpl::HeaderMapImpl() { memset(&inline_headers_, 0, sizeof(inline_headers_)); }

HeaderMapImpl::HeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl() {
rhs.iterate(
[](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
// TODO(mattklein123) PERF: Avoid copying here is not necessary.
HeaderString key_string;
key_string.setCopy(header.key().c_str(), header.key().size());
HeaderString value_string;
value_string.setCopy(header.value().c_str(), header.value().size());
HeaderMapImpl::HeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl() { copyFrom(rhs); }

static_cast<HeaderMapImpl*>(context)->addViaMove(std::move(key_string),
std::move(value_string));
return HeaderMap::Iterate::Continue;
},
this);
}
HeaderMapImpl::HeaderMapImpl(const HeaderMapImpl& rhs)

@mattklein123 mattklein123 Aug 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this change still needed? It's unclear to me why the compiler won't just pick the other one.

@htuch htuch Aug 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think so; the implicit copy constructor is deleted, the above constructor for HeaderMap isn't actually a copy constructor.

@mattklein123 mattklein123 Aug 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. OK.

: HeaderMapImpl(static_cast<const HeaderMap&>(rhs)) {}

@mattklein123 mattklein123 Aug 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry I'm confused. How is this different from what the compiler did before?

@htuch htuch Aug 13, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was synthesizing the default copy constructor, since we only supplied a constructor for the class we were deriving from AFAICT.

@htuch htuch Aug 13, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's something more going on here though, witness the build failures. Looking into this now.


HeaderMapImpl::HeaderMapImpl(
const std::initializer_list<std::pair<LowerCaseString, std::string>>& values)
Expand All @@ -304,6 +293,22 @@ HeaderMapImpl::HeaderMapImpl(
}
}

void HeaderMapImpl::copyFrom(const HeaderMap& header_map) {
header_map.iterate(

@jmarantz jmarantz Aug 14, 2018

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 defining a copy-ctor I think you should also define an assignment operator (and test it).

https://en.cppreference.com/w/cpp/language/rule_of_three

I see you are defining one in a test subclass, but it doesn't look like you've got one in HeaderMapImpl.

@htuch htuch Aug 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, that's fair. I will mvoe copy assignment to HeaderMapImpl.

[](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
// TODO(mattklein123) PERF: Avoid copying here is not necessary.

@jmarantz jmarantz Aug 14, 2018

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.

s/is/if/ ?

Do we keep a bit indicating whether a header-map contains owned strings or refs?

@htuch htuch Aug 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Will fix typo, performance optimization is orthogonal.

HeaderString key_string;
key_string.setCopy(header.key().c_str(), header.key().size());
HeaderString value_string;
value_string.setCopy(header.value().c_str(), header.value().size());

@jmarantz jmarantz Aug 14, 2018

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.

Best practice is to use .data() rather than .c_str() here (x2), although I admit in practice I don't see how they can differ :)

@htuch htuch Aug 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There is no .data() on header.value().


static_cast<HeaderMapImpl*>(context)->addViaMove(std::move(key_string),
std::move(value_string));
return HeaderMap::Iterate::Continue;
},
this);
}

bool HeaderMapImpl::operator==(const HeaderMapImpl& rhs) const {
if (size() != rhs.size()) {
return false;
Expand Down
10 changes: 7 additions & 3 deletions source/common/http/header_map_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,10 @@ class HeaderMapImpl : public HeaderMap {
static void appendToHeader(HeaderString& header, absl::string_view data);

HeaderMapImpl();
HeaderMapImpl(const std::initializer_list<std::pair<LowerCaseString, std::string>>& values);
HeaderMapImpl(const HeaderMap& rhs);
explicit HeaderMapImpl(
const std::initializer_list<std::pair<LowerCaseString, std::string>>& values);
explicit HeaderMapImpl(const HeaderMap& rhs);
HeaderMapImpl(const HeaderMapImpl& rhs);

/**
* Add a header via full move. This is the expected high performance paths for codecs populating
Expand Down Expand Up @@ -80,6 +82,8 @@ class HeaderMapImpl : public HeaderMap {
size_t size() const override { return headers_.size(); }

protected:
void copyFrom(const HeaderMap& rhs);

@jmarantz jmarantz Aug 14, 2018

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.

comment that these are protected because they are suspected to be slow, and should only be used in tests?


struct HeaderEntryImpl : public HeaderEntry, NonCopyable {
HeaderEntryImpl(const LowerCaseString& key);
HeaderEntryImpl(const LowerCaseString& key, HeaderString&& value);
Expand Down Expand Up @@ -131,7 +135,7 @@ class HeaderMapImpl : public HeaderMap {
* List of HeaderEntryImpl that keeps the pseudo headers (key starting with ':') in the front
* of the list (as required by nghttp2) and otherwise maintains insertion order.
*/
class HeaderList {
class HeaderList : NonCopyable {

@mattklein123 mattklein123 Aug 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fine to block move, but why not just block the implicit move constructor inside NonCopyable? I realize it's called NonCopyable but I'm pretty sure in all cases where we use that we also don't want move?

@mattklein123 mattklein123 Aug 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this change is needed anymore, but fine to keep.

public:
HeaderList() : pseudo_headers_end_(headers_.end()) {}

Expand Down
17 changes: 17 additions & 0 deletions test/common/http/header_map_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,5 +853,22 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) {
}
}

// Validate that [Test]HeaderMapImpl copy construction works. This is a
// regression for where we were missing a valid copy constructor.
TEST(HeaderMapImplTest, HeaderMapImplyCopy) {
{
HeaderMapImpl foo;
foo.addCopy(LowerCaseString("foo"), "bar");
auto headers = std::make_unique<HeaderMapImpl>(foo);
EXPECT_STREQ("bar", headers->get(LowerCaseString("foo"))->value().c_str());
}
{
TestHeaderMapImpl foo;
foo.addCopy("foo", "bar");
auto headers = std::make_unique<TestHeaderMapImpl>(foo);
EXPECT_EQ("bar", headers->get_("foo"));
}
}

} // namespace Http
} // namespace Envoy
11 changes: 11 additions & 0 deletions test/test_common/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,17 @@ TestHeaderMapImpl::TestHeaderMapImpl(

TestHeaderMapImpl::TestHeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl(rhs) {}

TestHeaderMapImpl& TestHeaderMapImpl::operator=(const TestHeaderMapImpl& other) {
if (&other == this) {
return *this;
}

removePrefix(LowerCaseString(""));

@jmarantz jmarantz Aug 14, 2018

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.

WDYT of factoring out a separate clear() method in the public API while you are in here?

@htuch htuch Aug 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I would rather not make any API public on HeaderMapImpl that we don't need to. This is a performance sensitive object and we should avoid having folks do slow/inefficient things with it that they don't need to.

@jmarantz jmarantz Aug 14, 2018

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 someone does need to clear(), I think it'd be better for it to be abstracted at the class interface.
Right now removePrefix(LowerCaseString("")) seems like it might be specific to the implementation?

But a compromise might be to define clear() but make it private or protected, with a note about exposing it in the future if there's a real need.

@htuch htuch Aug 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Arguably, nobody should be doing this, removePrefix is a horrible way to clear, it's only suitable for test purposes. I'm going to follow Matt's suggestion and move this to TestHeaderMapImpl only.

copyFrom(other);

return *this;
}

void TestHeaderMapImpl::addCopy(const std::string& key, const std::string& value) {
addCopy(LowerCaseString(key), value);
}
Expand Down
7 changes: 5 additions & 2 deletions test/test_common/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,11 @@ namespace Http {
class TestHeaderMapImpl : public HeaderMapImpl {
public:
TestHeaderMapImpl();
TestHeaderMapImpl(const std::initializer_list<std::pair<std::string, std::string>>& values);
TestHeaderMapImpl(const HeaderMap& rhs);
explicit TestHeaderMapImpl(
const std::initializer_list<std::pair<std::string, std::string>>& values);
explicit TestHeaderMapImpl(const HeaderMap& rhs);

TestHeaderMapImpl& operator=(const TestHeaderMapImpl& other);

friend std::ostream& operator<<(std::ostream& os, const TestHeaderMapImpl& p) {
p.iterate(
Expand Down