-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Improve time complexity of removal of callback handle. #11751
Changes from 27 commits
959fd1f
41e092a
3eb115b
1bc6ad1
e80b2a5
dc16f65
e95fc7c
89fc19f
2c9a3b8
b03decc
eb5ed70
1f211a3
a2e7ffd
72cc8ac
d0354cb
2d9a9fa
4c97b8d
956b32a
4f1bd5f
d859a2f
5739de5
7c4bf5a
e089eb8
6dc2de4
3e5ceaa
ccd18e6
0d0a7d6
26de9e3
d75466a
a850f6d
e8afadd
e227506
7c62189
d11b19a
b1ba481
b2ce936
fbc6a7d
85b84f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ template <typename... CallbackArgs> class CallbackManager { | |
*/ | ||
CallbackHandle* add(Callback callback) { | ||
callbacks_.emplace_back(*this, callback); | ||
callbacks_.back().it_ = (--callbacks_.end()); | ||
return &callbacks_.back(); | ||
} | ||
|
||
|
@@ -46,24 +47,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_); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}; | ||
|
||
/** | ||
* 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_; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,9 +84,7 @@ InlineScopedRoutesConfigProvider::InlineScopedRoutesConfigProvider( | |
ConfigProvider::ApiType::Delta), | ||
name_(std::move(name)), | ||
config_(std::make_shared<ScopedConfigImpl>(std::move(scope_key_builder))), | ||
config_protos_(std::make_move_iterator(config_protos.begin()), | ||
std::make_move_iterator(config_protos.end())), | ||
rds_config_source_(std::move(rds_config_source)) {} | ||
config_protos_(move(config_protos)), rds_config_source_(std::move(rds_config_source)) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you remove the unrelated optimization fixups and put them in a separate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree that these changes should be in another PR. |
||
|
||
ScopedRdsConfigSubscription::ScopedRdsConfigSubscription( | ||
const envoy::extensions::filters::network::http_connection_manager::v3::ScopedRds& scoped_rds, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -286,7 +286,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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
|
@@ -349,7 +349,7 @@ 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_EQ(m_per_cluster, 36827); | ||
EXPECT_MEMORY_LE(m_per_cluster, 38000); // Round up to allow platform variations. | ||
} | ||
|
||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.