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

Jump to bottom

Improve time complexity of removal of callback handle. #11751

Merged
merged 38 commits into from
Jul 7, 2020
Merged

Improve time complexity of removal of callback handle. #11751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
959fd1f
api: rename num_retries to max_retries (#11729)
numerodix Jun 24, 2020
41e092a
test: Split huge monolith mock header to speed up test compilation (#…
foreseeable Jun 25, 2020
3eb115b
build: removing hard-coded extension from generate_extension_db.py (…
alyssawilk Jun 25, 2020
1bc6ad1
Windows: test compilation refresh (#11719)
sunjayBhatia Jun 25, 2020
e80b2a5
Signed-off-by: chaoqinli <chaoqinli@google.com>
Jun 24, 2020
dc16f65
make cast dynamic
Jun 25, 2020
e95fc7c
Signed-off-by: chaoqinli <chaoqinli@google.com>
Jun 24, 2020
89fc19f
make cast dynamic
Jun 25, 2020
2c9a3b8
examples: fixed fault-injection delay demo (#11715)
cpakulski Jun 25, 2020
b03decc
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jun 25, 2020
eb5ed70
format
Jun 25, 2020
1f211a3
format
Jun 25, 2020
a2e7ffd
format
Jun 25, 2020
72cc8ac
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jun 25, 2020
d0354cb
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jun 26, 2020
2d9a9fa
rollback sth
Jun 26, 2020
4c97b8d
rollback sth
Jun 26, 2020
956b32a
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jun 26, 2020
4f1bd5f
rollback sth
Jun 26, 2020
d859a2f
rollback sth
Jun 26, 2020
5739de5
rollback sth
Jun 26, 2020
7c4bf5a
rollback sth
Jun 26, 2020
e089eb8
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jun 27, 2020
6dc2de4
rollback sth
Jun 27, 2020
3e5ceaa
rollback sth
Jun 27, 2020
ccd18e6
increase memory upper bound
Jun 27, 2020
0d0a7d6
increase exact memory consumed
Jun 27, 2020
26de9e3
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jun 29, 2020
d75466a
add comment of stats memory test
Jun 29, 2020
a850f6d
add comments, remove unrelated fixups
Jun 30, 2020
e8afadd
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jun 30, 2020
e227506
add comments, remove unrelated fixups
Jun 30, 2020
7c62189
fix format
Jun 30, 2020
d11b19a
fix format
Jun 30, 2020
b1ba481
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jul 1, 2020
b2ce936
fix format
Jul 1, 2020
fbc6a7d
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jul 3, 2020
85b84f4
Merge branch 'master' of https://github.com/envoyproxy/envoy into con…
Jul 6, 2020
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
18 changes: 9 additions & 9 deletions source/common/common/callback_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ template <typename... CallbackArgs> class CallbackManager {
*/
CallbackHandle* add(Callback callback) {
callbacks_.emplace_back(*this, callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks good. can you move this into the constructor? caller doesnt have to modify the handle outside of its box

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what is going on, I will pull from upstream to retest it first, it the same error show up again, I will test the change to find the bug.

// get the list iterator of added callback handle, which will be used to remove itself from
// callbacks_ list.
callbacks_.back().it_ = (--callbacks_.end());
return &callbacks_.back();
}

Expand All @@ -46,24 +49,21 @@ template <typename... CallbackArgs> class CallbackManager {
CallbackHolder(CallbackManager& parent, Callback cb) : parent_(parent), cb_(cb) {}

// CallbackHandle
void remove() override { parent_.remove(this); }
void remove() override { parent_.remove(it_); }
Copy link
Member

Choose a reason for hiding this comment

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

So, I think the safety of this depends on:

"Adding, removing and moving the elements within the list or across several lists does not invalidate the iterators or references. An iterator is invalidated only when the corresponding element is deleted."

in https://en.cppreference.com/w/cpp/container/list. I.e. that this is a std::list. I'd add some comments to the call sites here and where it_ is initialized, as well as to where callbacks_ is defined, to make sure that this is clear as an invariant.

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 guaranteed by C++ standard that list iterator won't be invalidated by list operation.


CallbackManager& parent_;
Callback cb_;

// the iterator of this callback holder inside callbacks_ list
// upon removal, use this iterator to delete callback holder in O(1)
typename std::list<CallbackHolder>::iterator it_;
Copy link
Contributor

Choose a reason for hiding this comment

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

have you considered to use a hashmap which may make the code a little easier to read?

Copy link
Member Author

@chaoqin-li1123 chaoqin-li1123 Jun 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am worried that a hashmap may incur more memory consumption and performance penalty. Also, storing a list iterator make the code less verbose without all the hashing and searching. As suggested, I have added some comment to make the code more readable.

Copy link
Member

Choose a reason for hiding this comment

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

I think what is done here is fine, as it's basically just std::list iterator semantics, which could be explained a bit better but are fairly clear.

};

/**
* Remove a member update callback added via add().
* @param handle supplies the callback handle to remove.
*/
void remove(CallbackHandle* handle) {
ASSERT(std::find_if(callbacks_.begin(), callbacks_.end(),
[handle](const CallbackHolder& holder) -> bool {
return handle == &holder;
}) != callbacks_.end());
callbacks_.remove_if(
[handle](const CallbackHolder& holder) -> bool { return handle == &holder; });
}
void remove(typename std::list<CallbackHolder>::iterator& it) { callbacks_.erase(it); }

std::list<CallbackHolder> callbacks_;
};
Expand Down
10 changes: 7 additions & 3 deletions test/integration/stats_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
// 2020/05/20 11223 44491 44600 Add primary clusters tracking to cluster manager.
// 2020/06/10 11561 44491 44811 Make upstreams pluggable
// 2020/04/23 10661 44425 46000 per-listener connection limits
// 2020/06/29 11751 44715 46000 Improve time complexity of removing callback handle
// in callback manager.

// Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI
// 'release' builds, where we control the platform and tool-chain. So you
Expand All @@ -287,7 +289,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
// If you encounter a failure here, please see
// https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests
// for details on how to fix.
EXPECT_MEMORY_EQ(m_per_cluster, 44491);
EXPECT_MEMORY_EQ(m_per_cluster, 44715);
EXPECT_MEMORY_LE(m_per_cluster, 46000); // Round up to allow platform variations.
Copy link
Contributor

Choose a reason for hiding this comment

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

could you update the comment section above and explain a little bit why this bump is necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the comment. If there are a lot of callbacks in a callback manager or if adding and removing callbacks is frequent, making removal of callback handle O(1) will be a good trade off because storing a iterator is lightweight. It will make the memory consumption of a callback holder increase from 48 bytes to 56 bytes in a 64 bit linux system, but will be likely to improve performance when callback is used frequently.

}

Expand Down Expand Up @@ -338,6 +340,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) {
// 2020/05/20 11223 36603 36800 Add primary clusters tracking to cluster manager.
// 2020/06/10 11561 36603 36923 Make upstreams pluggable
// 2020/04/23 10661 36537 37000 per-listener connection limits
// 2020/06/29 11751 36827 38000 Improve time complexity of removing callback handle.
// in callback manager.

// Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI
// 'release' builds, where we control the platform and tool-chain. So you
Expand All @@ -351,8 +355,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) {
// If you encounter a failure here, please see
// https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests
// for details on how to fix.
EXPECT_MEMORY_EQ(m_per_cluster, 36603);
EXPECT_MEMORY_LE(m_per_cluster, 37000);
EXPECT_MEMORY_EQ(m_per_cluster, 36827);
EXPECT_MEMORY_LE(m_per_cluster, 38000); // Round up to allow platform variations.
}

TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) {
Expand Down