From bf330a99f043c07e9b77939cd6bb328d388a427b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Dziepak?= Date: Fri, 16 Mar 2018 13:18:13 +0000 Subject: [PATCH] mutation_partition_view: pass cell by value to visitor mutation_partition_view needs to create an atomic_cell from IDL-serialised data. Then that cell is passed to the visitor. However, because generic mutation_partition_visitor interface was used, the cell was passed by constant reference which forced the visitor to needlessly copy it. This patch takes advantage of the fact that mutation_partition_view is devirtualised now and adjust the interfaces of its visitors so that the cell can be passed without copying. --- converting_mutation_partition_applier.hh | 8 +++++++ mutation_partition_view.cc | 27 ++++++++---------------- mutation_partition_view.hh | 6 +++--- partition_builder.hh | 18 +++++++++++----- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/converting_mutation_partition_applier.hh b/converting_mutation_partition_applier.hh index 81b4b3d3fd..d05f429f73 100644 --- a/converting_mutation_partition_applier.hh +++ b/converting_mutation_partition_applier.hh @@ -92,6 +92,10 @@ public: _p.apply(t); } + void accept_static_cell(column_id id, atomic_cell cell) { + return accept_static_cell(id, atomic_cell_view(cell)); + } + virtual void accept_static_cell(column_id id, atomic_cell_view cell) override { const column_mapping_entry& col = _visited_column_mapping.static_column_at(id); const column_definition* def = _p_schema.get_column_definition(col.name()); @@ -119,6 +123,10 @@ public: _current_row = &r; } + void accept_row_cell(column_id id, atomic_cell cell) { + return accept_row_cell(id, atomic_cell_view(cell)); + } + virtual void accept_row_cell(column_id id, atomic_cell_view cell) override { const column_mapping_entry& col = _visited_column_mapping.regular_column_at(id); const column_definition* def = _p_schema.get_column_definition(col.name()); diff --git a/mutation_partition_view.cc b/mutation_partition_view.cc index 46186fcffb..a750c0e3d7 100644 --- a/mutation_partition_view.cc +++ b/mutation_partition_view.cc @@ -134,14 +134,7 @@ void read_and_visit_row(ser::row_view rv, const column_mapping& cm, column_kind if (!_col.type()->is_atomic()) { throw std::runtime_error("A collection expected, got an atomic cell"); } - // FIXME: Pass view to cell to avoid copy - auto&& outer = current_allocator(); - with_allocator(standard_allocator(), [&] { - auto cell = read_atomic_cell(*_col.type(), acv); - with_allocator(outer, [&] { - _visitor.accept_atomic_cell(_id, cell); - }); - }); + _visitor.accept_atomic_cell(_id, read_atomic_cell(*_col.type(), acv)); } void operator()(ser::collection_cell_view& ccv) const { if (_col.type()->is_atomic()) { @@ -200,8 +193,8 @@ void mutation_partition_view::do_accept(const column_mapping& cm, Visitor& visit struct static_row_cell_visitor { Visitor& _visitor; - void accept_atomic_cell(column_id id, const atomic_cell& ac) const { - _visitor.accept_static_cell(id, ac); + void accept_atomic_cell(column_id id, atomic_cell ac) const { + _visitor.accept_static_cell(id, std::move(ac)); } void accept_collection(column_id id, const collection_mutation& cm) const { _visitor.accept_static_cell(id, cm); @@ -220,8 +213,8 @@ void mutation_partition_view::do_accept(const column_mapping& cm, Visitor& visit struct cell_visitor { Visitor& _visitor; - void accept_atomic_cell(column_id id, const atomic_cell& ac) const { - _visitor.accept_row_cell(id, ac); + void accept_atomic_cell(column_id id, atomic_cell ac) const { + _visitor.accept_row_cell(id, std::move(ac)); } void accept_collection(column_id id, const collection_mutation& cm) const { _visitor.accept_row_cell(id, cm); @@ -280,9 +273,8 @@ mutation_fragment frozen_mutation_fragment::unfreeze(const schema& s) public: clustering_row_builder(const schema& s, clustering_key key, row_tombstone t, row_marker m) : _s(s), _mf(mutation_fragment::clustering_row_tag_t(), std::move(key), std::move(t), std::move(m), row()) { } - void accept_atomic_cell(column_id id, const atomic_cell& ac) { - auto& type = *_s.regular_column_at(id).type; - _mf.as_mutable_clustering_row().cells().append_cell(id, atomic_cell_or_collection(atomic_cell(type, ac))); + void accept_atomic_cell(column_id id, atomic_cell ac) { + _mf.as_mutable_clustering_row().cells().append_cell(id, atomic_cell_or_collection(std::move(ac))); } void accept_collection(column_id id, const collection_mutation& cm) { auto& ctype = *static_pointer_cast(_s.regular_column_at(id).type); @@ -303,9 +295,8 @@ mutation_fragment frozen_mutation_fragment::unfreeze(const schema& s) mutation_fragment _mf; public: explicit static_row_builder(const schema& s) : _s(s), _mf(static_row()) { } - void accept_atomic_cell(column_id id, const atomic_cell& ac) { - auto& type = *_s.static_column_at(id).type; - _mf.as_mutable_static_row().cells().append_cell(id, atomic_cell_or_collection(atomic_cell(type, ac))); + void accept_atomic_cell(column_id id, atomic_cell ac) { + _mf.as_mutable_static_row().cells().append_cell(id, atomic_cell_or_collection(std::move(ac))); } void accept_collection(column_id id, const collection_mutation& cm) { auto& ctype = *static_pointer_cast(_s.static_column_at(id).type); diff --git a/mutation_partition_view.hh b/mutation_partition_view.hh index 6e2aada995..15874e879b 100644 --- a/mutation_partition_view.hh +++ b/mutation_partition_view.hh @@ -34,17 +34,17 @@ class converting_mutation_partition_applier; GCC6_CONCEPT( template -concept bool MutationViewVisitor = requires (T visitor, tombstone t, atomic_cell_view acv, +concept bool MutationViewVisitor = requires (T visitor, tombstone t, atomic_cell ac, collection_mutation_view cmv, range_tombstone rt, position_in_partition_view pipv, row_tombstone row_tomb, row_marker rm) { visitor.accept_partition_tombstone(t); - visitor.accept_static_cell(column_id(), acv); + visitor.accept_static_cell(column_id(), std::move(ac)); visitor.accept_static_cell(column_id(), cmv); visitor.accept_row_tombstone(rt); visitor.accept_row(pipv, row_tomb, rm, is_dummy::no, is_continuous::yes); - visitor.accept_row_cell(column_id(), acv); + visitor.accept_row_cell(column_id(), std::move(ac)); visitor.accept_row_cell(column_id(), cmv); }; ) diff --git a/partition_builder.hh b/partition_builder.hh index 7ef51856c7..ea5d744bc1 100644 --- a/partition_builder.hh +++ b/partition_builder.hh @@ -25,7 +25,7 @@ #include "mutation_partition_view.hh" // Partition visitor which builds mutation_partition corresponding to the data its fed with. -class partition_builder : public mutation_partition_visitor { +class partition_builder final : public mutation_partition_visitor { private: const schema& _schema; mutation_partition& _partition; @@ -43,9 +43,13 @@ public: } virtual void accept_static_cell(column_id id, atomic_cell_view cell) override { - row& r = _partition.static_row(); auto& cdef = _schema.static_column_at(id); - r.append_cell(id, atomic_cell_or_collection(*cdef.type, cell)); + accept_static_cell(id, atomic_cell(*cdef.type, cell)); + } + + void accept_static_cell(column_id id, atomic_cell&& cell) { + row& r = _partition.static_row(); + r.append_cell(id, atomic_cell_or_collection(std::move(cell))); } virtual void accept_static_cell(column_id id, collection_mutation_view collection) override { @@ -66,9 +70,13 @@ public: } virtual void accept_row_cell(column_id id, atomic_cell_view cell) override { - row& r = _current_row->cells(); auto& cdef = _schema.regular_column_at(id); - r.append_cell(id, atomic_cell_or_collection(*cdef.type, cell)); + accept_row_cell(id, atomic_cell(*cdef.type, cell)); + } + + void accept_row_cell(column_id id, atomic_cell&& cell) { + row& r = _current_row->cells(); + r.append_cell(id, atomic_cell_or_collection(std::move(cell))); } virtual void accept_row_cell(column_id id, collection_mutation_view collection) override {