From ea78fa29ae1ce8a359e7c5a3bee1427ab48ebde9 Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Fri, 18 Oct 2019 00:22:44 +0200 Subject: [PATCH] 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 --- src/crimson/osd/ops_executer.h | 6 +++++ src/crimson/osd/pg.cc | 43 ++++++++++++++++------------------ 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/crimson/osd/ops_executer.h b/src/crimson/osd/ops_executer.h index 2606bb81908..0696443501e 100644 --- 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 f270d266984..65723aa39cb 100644 --- a/src/crimson/osd/pg.cc +++ b/src/crimson/osd/pg.cc @@ -439,22 +439,21 @@ seastar::future> PG::do_osd_ops(Ref 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(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> PG::do_osd_ops(Ref m) seastar::future> PG::do_pg_ops(Ref 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(*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(m.get(), 0, get_osdmap_epoch(), CEPH_OSD_FLAG_ACK | CEPH_OSD_FLAG_ONDISK, false); -- 2.39.5