From: Radoslaw Zarzynski Date: Sun, 16 Sep 2018 15:33:50 +0000 (+0200) Subject: common, osd: kill OpQueue::length() to minimize inter-core traffic. X-Git-Tag: v14.1.0~727^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=65f25b7f138cfc0b961dad45400aebed5731ef7e;p=ceph.git common, osd: kill OpQueue::length() to minimize inter-core traffic. Before ------ ``` - 1,50% 1,34% msgr-worker-0 ceph-osd [.] WeightedPriorityQueue::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::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 --- diff --git a/src/common/OpQueue.h b/src/common/OpQueue.h index 26894b37fd2a..db98cbd318f9 100644 --- a/src/common/OpQueue.h +++ b/src/common/OpQueue.h @@ -35,8 +35,6 @@ template 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 2e86d1069ff4..6d7de1291f66 100644 --- 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 91e16ea2aa5b..a05174e8185d 100644 --- a/src/common/WeightedPriorityQueue.h +++ b/src/common/WeightedPriorityQueue.h @@ -104,19 +104,16 @@ class WeightedPriorityQueue : public OpQueue unsigned get_size() const { return lp.size(); } - unsigned filter_class(std::list* out) { - unsigned count = 0; + void filter_class(std::list* out) { for (Lit i = --lp.end();; --i) { if (out) { out->push_front(std::move(i->item)); } i = lp.erase_and_dispose(i, DelItem()); - ++count; if (i == lp.begin()) { break; } } - return count; } }; class SubQueue : public bi::set_base_hook<> @@ -172,17 +169,23 @@ class WeightedPriorityQueue : public OpQueue check_end(); return ret; } - unsigned filter_class(K& cl, std::list* out) { - unsigned count = 0; + void filter_class(K& cl, std::list* out) { Kit i = klasses.find(cl, MapKey()); if (i != klasses.end()) { - count = i->filter_class(out); + i->filter_class(out); Kit tmp = klasses.erase_and_dispose(i, DelItem()); 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 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()); } 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 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 } void filter_class(K& cl, std::list* 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()); @@ -278,6 +277,14 @@ class WeightedPriorityQueue : public OpQueue } } } + // 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 { std::srand(time(0)); } - unsigned length() const final { - return strict.size + normal.size; - } void remove_by_class(K cl, std::list* 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 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 5a87de35f515..d36ea202bad6 100644 --- 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 b8cfa0cb7571..7b1e494a3f54 100644 --- 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 *out) override final { diff --git a/src/osd/mClockOpClassQueue.h b/src/osd/mClockOpClassQueue.h index 30074079c80b..c2ec06aa417b 100644 --- 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 *out) override final { diff --git a/src/test/common/test_weighted_priority_queue.cc b/src/test/common/test_weighted_priority_queue.cc index 9bb87177147c..263fc4cb4d65 100644 --- 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));