common, osd: kill OpQueue::length() to minimize inter-core traffic.

Before
------
```
-    1,50%     1,34%  msgr-worker-0  ceph-osd  [.] WeightedPriorityQueue<OpQueueItem, unsigned long>::Queue::insert
```

Where self if 1.34%
```
       │       mov    %r12d,0x2c(%r14)
       │               }
       │               ++size;
 17,33 │1ba:   addl   $0x1,0x2ec48(%r14)
       │             }
```

After
-----
```
-    1,05%     0,93%  msgr-worker-0  ceph-osd  [.] WeightedPriorityQueue<OpQueueItem, unsigned long>::Queue::insert
```

Let's annotate hottest place (self is 0,93%):
```
       │           //Find the upper bound, cache the previous value and if we should
       │           //store it in the left or right node
       │           bool left_child = true;
       │           while(x){
       │ a0:   mov    %rax,%rbx
       │              ++depth;
       │              y = x;
       │              x = (left_child = comp(key, x)) ?
  8,86 │ a3:   cmp    0x20(%rbx),%r15
       │     _ZN5boost9intrusive31default_rbtree_node_traits_implIPvE9get_rightERKPNS0_11r
```

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

diff --git a/src/common/OpQueue.h b/src/common/OpQueue.h
index 26894b37fd2a3366e748b74494eaac77d52bf0fe..db98cbd318f912d27da246a18570358e5a510419 100644 (file)
--- a/src/common/OpQueue.h
+++ b/src/common/OpQueue.h
@@ -35,8 +35,6 @@ template <typename T, typename K>
 class OpQueue {
 
   public:
-    // How many Ops are in the queue
-    virtual unsigned length() const = 0;
     // Ops of this class should be deleted immediately. If out isn't
     // nullptr then items should be added to the front in
     // front-to-back order. The typical strategy is to visit items in

diff --git a/src/common/PrioritizedQueue.h b/src/common/PrioritizedQueue.h
index 2e86d1069ff45109f9fcd36aee3085106af09a74..6d7de1291f66de5ad66ddb208d053a63f204e394 100644 (file)
--- a/src/common/PrioritizedQueue.h
+++ b/src/common/PrioritizedQueue.h
@@ -202,7 +202,7 @@ public:
       min_cost(min_c)
   {}
 
-  unsigned length() const final {
+  unsigned length() const {
     unsigned total = 0;
     for (typename SubQueues::const_iterator i = queue.begin();
         i != queue.end();

diff --git a/src/common/WeightedPriorityQueue.h b/src/common/WeightedPriorityQueue.h
index 91e16ea2aa5be2561a3ecf3ea240402069a8dbcb..a05174e8185da34683111f3df0f0fc61a46b6894 100644 (file)
--- a/src/common/WeightedPriorityQueue.h
+++ b/src/common/WeightedPriorityQueue.h
@@ -104,19 +104,16 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
       unsigned get_size() const {
        return lp.size();
       }
-      unsigned filter_class(std::list<T>* out) {
-        unsigned count = 0;
+      void filter_class(std::list<T>* out) {
         for (Lit i = --lp.end();; --i) {
           if (out) {
             out->push_front(std::move(i->item));
           }
           i = lp.erase_and_dispose(i, DelItem<ListPair>());
-          ++count;
           if (i == lp.begin()) {
             break;
           }
         }
-        return count;
       }
     };
     class SubQueue : public bi::set_base_hook<>
@@ -172,17 +169,23 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
         check_end();
        return ret;
       }
-      unsigned filter_class(K& cl, std::list<T>* out) {
-       unsigned count = 0;
+      void filter_class(K& cl, std::list<T>* out) {
         Kit i = klasses.find(cl, MapKey<Klass, K>());
         if (i != klasses.end()) {
-          count = i->filter_class(out);
+          i->filter_class(out);
          Kit tmp = klasses.erase_and_dispose(i, DelItem<Klass>());
          if (next == i) {
             next = tmp;
           }
           check_end();
         }
+      }
+      // this is intended for unit tests and should be never used on hot paths
+      unsigned get_size_slow() const {
+       unsigned count = 0;
+       for (const auto& klass : klasses) {
+         count += klass.get_size();
+       }
        return count;
       }
       void dump(ceph::Formatter *f) const {
@@ -199,17 +202,15 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
       unsigned total_prio;
       unsigned max_cost;
       public:
-       unsigned size;
        Queue() :
          total_prio(0),
-         max_cost(0),
-         size(0) {
+         max_cost(0) {
        }
        ~Queue() {
          queues.clear_and_dispose(DelItem<SubQueue>());
        }
        bool empty() const {
-         return !size;
+         return queues.empty();
        }
        void insert(unsigned p, K cl, unsigned cost, T&& item, bool front = false) {
          typename SubQueues::insert_commit_data insert_data;
@@ -223,10 +224,8 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
          if (cost > max_cost) {
            max_cost = cost;
          }
-         ++size;
        }
        T pop(bool strict = false) {
-         --size;
          Sit i = --queues.end();
          if (strict) {
            T ret = i->pop();
@@ -269,7 +268,7 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
        }
        void filter_class(K& cl, std::list<T>* out) {
          for (Sit i = queues.begin(); i != queues.end();) {
-           size -= i->filter_class(cl, out);
+           i->filter_class(cl, out);
            if (i->empty()) {
              total_prio -= i->key;
              i = queues.erase_and_dispose(i, DelItem<SubQueue>());
@@ -278,6 +277,14 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
            }
          }
        }
+       // this is intended for unit tests and should be never used on hot paths
+       unsigned get_size_slow() const {
+         unsigned count = 0;
+         for (const auto& queue : queues) {
+           count += queue.get_size_slow();
+         }
+         return count;
+       }
        void dump(ceph::Formatter *f) const {
          for (typename SubQueues::const_iterator i = queues.begin();
                i != queues.end(); ++i) {
@@ -300,15 +307,12 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
       {
        std::srand(time(0));
       }
-    unsigned length() const final {
-      return strict.size + normal.size;
-    }
     void remove_by_class(K cl, std::list<T>* removed = 0) final {
       strict.filter_class(cl, removed);
       normal.filter_class(cl, removed);
     }
     bool empty() const final {
-      return !(strict.size + normal.size);
+      return strict.empty() && normal.empty();
     }
     void enqueue_strict(K cl, unsigned p, T&& item) final {
       strict.insert(p, cl, 0, std::move(item));
@@ -323,12 +327,15 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
       normal.insert(p, cl, cost, std::move(item), true);
     }
     T dequeue() override {
-      ceph_assert(strict.size + normal.size > 0);
+      ceph_assert(!empty());
       if (!strict.empty()) {
        return strict.pop(true);
       }
       return normal.pop();
     }
+    unsigned get_size_slow() {
+      return strict.get_size_slow() + normal.get_size_slow();
+    }
     void dump(ceph::Formatter *f) const override {
       f->open_array_section("high_queues");
       strict.dump(f);

diff --git a/src/common/mClockPriorityQueue.h b/src/common/mClockPriorityQueue.h
index 5a87de35f515a2ef7f20ced7663701d182be3be6..d36ea202bad6388d7a5c1f95f45e2b08ccb01686 100644 (file)
--- a/src/common/mClockPriorityQueue.h
+++ b/src/common/mClockPriorityQueue.h
@@ -68,7 +68,7 @@ namespace ceph {
       Classes q;
 
       unsigned tokens, max_tokens;
-      int64_t size;
+      int64_t size; // XXX: this is only for the sake of dump().
 
       typename Classes::iterator cur;
 
@@ -201,7 +201,7 @@ namespace ceph {
       }
 
       void dump(ceph::Formatter *f) const {
-       f->dump_int("size", size);
+       f->dump_int("size", length());
        f->dump_int("num_keys", q.size());
       }
     };
@@ -229,7 +229,8 @@ namespace ceph {
       // empty
     }
 
-    unsigned length() const override final {
+    // XXX: used only by the unitest?
+    unsigned length() const {
       unsigned total = 0;
       total += queue_front.size();
       total += queue.request_count();

diff --git a/src/osd/mClockClientQueue.h b/src/osd/mClockClientQueue.h
index b8cfa0cb75711753b8192dec6959db8af683e5fa..7b1e494a3f5462bca0cafa6891c2d7c38d602713 100644 (file)
--- a/src/osd/mClockClientQueue.h
+++ b/src/osd/mClockClientQueue.h
@@ -53,10 +53,6 @@ namespace ceph {
 
     const crimson::dmclock::ClientInfo* op_class_client_info_f(const InnerClient& client);
 
-    inline unsigned length() const override final {
-      return queue.length();
-    }
-
     // Ops of this priority should be deleted immediately
     inline void remove_by_class(Client cl,
                                std::list<Request> *out) override final {

diff --git a/src/osd/mClockOpClassQueue.h b/src/osd/mClockOpClassQueue.h
index 30074079c80b4ab7f4b5394bbb4b1cb604cda7b1..c2ec06aa417b7b5331340770d683c2ace8b5e913 100644 (file)
--- a/src/osd/mClockOpClassQueue.h
+++ b/src/osd/mClockOpClassQueue.h
@@ -52,10 +52,6 @@ namespace ceph {
     const crimson::dmclock::ClientInfo*
     op_class_client_info_f(const osd_op_type_t& op_type);
 
-    inline unsigned length() const override final {
-      return queue.length();
-    }
-
     // Ops of this priority should be deleted immediately
     inline void remove_by_class(Client cl,
                                std::list<Request> *out) override final {

diff --git a/src/test/common/test_weighted_priority_queue.cc b/src/test/common/test_weighted_priority_queue.cc
index 9bb87177147c69b30b128b8339e3f6749311e940..263fc4cb4d6594486f1cdfdc31edcc01ff8eab11 100644 (file)
--- a/src/test/common/test_weighted_priority_queue.cc
+++ b/src/test/common/test_weighted_priority_queue.cc
@@ -163,30 +163,30 @@ protected:
 TEST_F(WeightedPriorityQueueTest, wpq_size){
   WQ wq(0, 0);
   EXPECT_TRUE(wq.empty());
-  EXPECT_EQ(0u, wq.length());
+  EXPECT_EQ(0u, wq.get_size_slow());
 
   // Test the strict queue size.
   for (unsigned i = 1; i < 5; ++i) {
     wq.enqueue_strict(Klass(i),i, std::make_tuple(i, i, i));
     EXPECT_FALSE(wq.empty());
-    EXPECT_EQ(i, wq.length());
+    EXPECT_EQ(i, wq.get_size_slow());
   }
   // Test the normal queue size.
   for (unsigned i = 5; i < 10; ++i) {
     wq.enqueue(Klass(i), i, i, std::make_tuple(i, i, i));
     EXPECT_FALSE(wq.empty());
-    EXPECT_EQ(i, wq.length());
+    EXPECT_EQ(i, wq.get_size_slow());
   }
   // Test that as both queues are emptied
   // the size is correct.
   for (unsigned i = 8; i >0; --i) {
     wq.dequeue();
     EXPECT_FALSE(wq.empty());
-    EXPECT_EQ(i, wq.length());
+    EXPECT_EQ(i, wq.get_size_slow());
   }
   wq.dequeue();
   EXPECT_TRUE(wq.empty());
-  EXPECT_EQ(0u, wq.length());
+  EXPECT_EQ(0u, wq.get_size_slow());
 }
 
 TEST_F(WeightedPriorityQueueTest, wpq_test_static) {
@@ -228,7 +228,7 @@ TEST_F(WeightedPriorityQueueTest, wpq_test_remove_by_class) {
   wq.remove_by_class(k, &wq_removed);
   // Check that the right ops were removed.
   EXPECT_EQ(num_to_remove, wq_removed.size());
-  EXPECT_EQ(num_items - num_to_remove, wq.length());
+  EXPECT_EQ(num_items - num_to_remove, wq.get_size_slow());
   for (Removed::iterator it = wq_removed.begin();
        it != wq_removed.end(); ++it) {
     EXPECT_EQ(k, std::get<1>(*it));