Cherrypick callback manager improvement fix. (#263)

* Cherrypick callback manager improvement fix. * update stats integration test numbers. * update the comment for nu as well. Co-authored-by: chaoqin-li1123 <[email protected]>
istio · Sep 22, 2020 · a1ef7b2 · a1ef7b2
1 parent e5870c9
commit a1ef7b2
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 13 deletions.
diff --git a/source/common/common/callback_impl.h b/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_;
 };

diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc
@@ -274,6 +274,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
   // 2020/05/13  10531    44425       44600   Refactor resource manager
   // 2020/04/23  10661    44425       44600   per-listener connection limits
   // 2020/06/09  tbd      43541       44600   add stats and stream flush timeout
+  // 2020/06/29  11751    43765       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,8 +289,8 @@ 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, 43541);
-  EXPECT_MEMORY_LE(m_per_cluster, 44600);
+  EXPECT_MEMORY_EQ(m_per_cluster, 43765);
+  EXPECT_MEMORY_LE(m_per_cluster, 46000); // Round up to allow platform variations.
 }
 
 TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) {
@@ -335,6 +337,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) {
   // 2020/05/13  10531    36537       44600   Refactor resource manager
   // 2020/04/23  10661    36537       36800   per-listener connection limits
   // 2020/06/09  tbd      35749       36800   add stats and stream flush timeout
+  // 2020/06/29  11751    35973       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
@@ -348,8 +352,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, 35749);
-  EXPECT_MEMORY_LE(m_per_cluster, 36800);
+  EXPECT_MEMORY_EQ(m_per_cluster, 35973);
+  EXPECT_MEMORY_LE(m_per_cluster, 38000); // Round up to allow platform variations.
 }
 
 TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) {