crimson: avoid seastar::do_with() due to performance reasons.

`seastar::do_with(T&& rvalue, F&& f) takes object for lifetime
extension by rvalue reference. This imposes materialization of
a temporary to move from even when `do_with()` is being called
like:

  `do_with(OpsExecuter{...}, [] { /* ... */)`.

The reason behind that is following language rule:

  "Temporary objects are created when a prvalue is materialized
  so that it can be used as a glvalue, which occurs (since C++17)
  in the following situations:

   * binding a reference to a prvalue"
  (from: "Temporary object lifetime", cppreference.com)

As OpsExecuter is pretty heavy-weight, it is reasonable to avoid
`do_with()` and perform the lifetime extension with smart pointer.
Additional benefit is squeezing plain-to-errorated conversion in
`seastar::internal::do_with_state::get_future()`.

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

diff --git a/src/crimson/osd/ops_executer.h b/src/crimson/osd/ops_executer.h
index 2606bb8190817f7443095f71c538ddb4696c670f..0696443501e068f3ea9c16f50fb928c9c64a682a 100644 (file)
--- a/src/crimson/osd/ops_executer.h
+++ b/src/crimson/osd/ops_executer.h
@@ -48,6 +48,12 @@ class OpsExecuter {
   using get_attr_errorator = PGBackend::get_attr_errorator;
 
 public:
+  // because OpsExecuter is pretty heavy-weight object we want to ensure
+  // it's not copied nor even moved by accident. Performance is the sole
+  // reason for prohibiting that.
+  OpsExecuter(OpsExecuter&&) = delete;
+  OpsExecuter(const OpsExecuter&) = delete;
+
   using osd_op_errorator = crimson::compound_errorator_t<
     call_errorator,
     read_errorator,

diff --git a/src/crimson/osd/pg.cc b/src/crimson/osd/pg.cc
index f270d2669848747f9d13808f0e9cb6ce191c71e6..65723aa39cb31bfc7fa13daf26a7e9dc79fd0a1f 100644 (file)
--- a/src/crimson/osd/pg.cc
+++ b/src/crimson/osd/pg.cc
@@ -439,22 +439,21 @@ seastar::future<Ref<MOSDOpReply>> PG::do_osd_ops(Ref<MOSDOp> m)
   const auto oid = m->get_snapid() == CEPH_SNAPDIR ? m->get_hobj().get_head()
                                                    : m->get_hobj();
   return backend->get_object_state(oid).safe_then([this, m](auto os) mutable {
-    return seastar::do_with(OpsExecuter{std::move(os), *this/* as const& */, m},
-                         [this, m] (auto& ox) {
-      return crimson::do_for_each(m->ops, [this, &ox](OSDOp& osd_op) {
-        logger().debug("will be handling op {}", ceph_osd_op_name(osd_op.op.op));
-        return ox.execute_osd_op(osd_op);
-      }).safe_then([this, m, &ox] {
-        logger().debug("all operations have been executed successfully");
-        return std::move(ox).submit_changes([this, m] (auto&& txn, auto&& os) -> osd_op_errorator::future<> {
-          // XXX: the entire lambda could be scheduled conditionally. ::if_then()?
-         if (txn.empty()) {
-            logger().debug("txn is empty, bypassing mutate");
-           return osd_op_errorator::now();
-         } else {
-           return submit_transaction(std::move(os), std::move(txn), *m);
-         }
-        });
+    auto ox =
+      std::make_unique<OpsExecuter>(std::move(os), *this/* as const& */, m);
+    return crimson::do_for_each(m->ops, [this, ox = ox.get()](OSDOp& osd_op) {
+      logger().debug("will be handling op {}", ceph_osd_op_name(osd_op.op.op));
+      return ox->execute_osd_op(osd_op);
+    }).safe_then([this, m, ox = std::move(ox)] {
+      logger().debug("all operations have been executed successfully");
+      return std::move(*ox).submit_changes([this, m] (auto&& txn, auto&& os) -> osd_op_errorator::future<> {
+        // XXX: the entire lambda could be scheduled conditionally. ::if_then()?
+        if (txn.empty()) {
+          logger().debug("txn is empty, bypassing mutate");
+          return osd_op_errorator::now();
+        } else {
+          return submit_transaction(std::move(os), std::move(txn), *m);
+        }
       });
     });
   }).safe_then([m,this] {
@@ -489,13 +488,11 @@ seastar::future<Ref<MOSDOpReply>> PG::do_osd_ops(Ref<MOSDOp> m)
 
 seastar::future<Ref<MOSDOpReply>> PG::do_pg_ops(Ref<MOSDOp> m)
 {
-  return seastar::do_with(OpsExecuter{*this/* as const& */, m},
-                          [this, m] (auto& ox) {
-    return seastar::do_for_each(m->ops, [this, &ox](OSDOp& osd_op) {
-      logger().debug("will be handling pg op {}", ceph_osd_op_name(osd_op.op.op));
-      return ox.execute_pg_op(osd_op);
-    });
-  }).then([m, this] {
+  auto ox = std::make_unique<OpsExecuter>(*this/* as const& */, m);
+  return seastar::do_for_each(m->ops, [this, ox = ox.get()](OSDOp& osd_op) {
+    logger().debug("will be handling pg op {}", ceph_osd_op_name(osd_op.op.op));
+    return ox->execute_pg_op(osd_op);
+  }).then([m, this, ox = std::move(ox)] {
     auto reply = make_message<MOSDOpReply>(m.get(), 0, get_osdmap_epoch(),
                                            CEPH_OSD_FLAG_ACK | CEPH_OSD_FLAG_ONDISK,
                                            false);