Improve time complexity of removal of callback handle. (#11751)

Improve the time complexity of removal of a callback handle from O(N) to O(1) by storing a list iterator to itself inside the callback_holder. Currently, when a callback handle is removed from the list, the pointer of the handle is used for linear search to locate the callback holder and delete it. Since list.erase() cost constant time if we provide the iterator, and list iterator don't get invalidated in other list operations. We can store the iterator inside the callback holder when we initialize it and use it for constant time removal.

Risk Level: Low
Testing: Pass all test in /test repo.
Fixes #11665

Signed-off-by: chaoqinli <chaoqinli@google.com>

source/common/common/callback_impl.h

@@ -24,6 +24,9 @@ template <typename... CallbackArgs> class CallbackManager {
  */
  CallbackHandle* add(Callback callback) {
  callbacks_.emplace_back(*this, callback);
+ // 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();
  }
 
@@ -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_); }
 
  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_;
  };
 
  /**
  * 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_;
 };

test/integration/stats_integration_test.cc

@@ -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
@@ -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.
 }
 
@@ -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
@@ -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) {