kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From wdberke...@apache.org
Subject kudu git commit: Move constructor and assignment operator for Schema
Date Fri, 05 Oct 2018 18:39:51 GMT
Repository: kudu
Updated Branches:
  refs/heads/master cf1b1f42c -> b848488d9


Move constructor and assignment operator for Schema

This adds a move constructor and move assignment operator to Schema.

I removed Schema::swap. It was only used in tests, and any potential
use of it should be adequately fulfilled by one of moves, copies, or
Schema::Reset.

Change-Id: I6c827c84657875be4cf2bc565db03a686849d8e2
Reviewed-on: http://gerrit.cloudera.org:8080/11593
Reviewed-by: Adar Dembo <adar@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <aserbin@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/b848488d
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/b848488d
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/b848488d

Branch: refs/heads/master
Commit: b848488d9bd2efcfc63ddc073c3addece33793b6
Parents: cf1b1f4
Author: Will Berkeley <wdberkeley@gmail.org>
Authored: Sun Sep 30 10:17:21 2018 -0700
Committer: Will Berkeley <wdberkeley@gmail.com>
Committed: Fri Oct 5 18:38:54 2018 +0000

----------------------------------------------------------------------
 src/kudu/common/id_mapping.h   | 13 ------
 src/kudu/common/schema-test.cc | 80 ++++++++++++++++++++++++++++---------
 src/kudu/common/schema.cc      | 49 ++++++++++++++++++-----
 src/kudu/common/schema.h       | 29 +++++++-------
 4 files changed, 115 insertions(+), 56 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/id_mapping.h
----------------------------------------------------------------------
diff --git a/src/kudu/common/id_mapping.h b/src/kudu/common/id_mapping.h
index a5d8289..f762b75 100644
--- a/src/kudu/common/id_mapping.h
+++ b/src/kudu/common/id_mapping.h
@@ -64,13 +64,6 @@ class IdMapping {
     clear();
   }
 
-  explicit IdMapping(const IdMapping& other)
-  : mask_(other.mask_),
-    entries_(other.entries_) {
-  }
-
-  ~IdMapping() {}
-
   void clear() {
     ClearMap(&entries_);
   }
@@ -84,12 +77,6 @@ class IdMapping {
     other.entries_.swap(entries_);
   }
 
-  IdMapping& operator=(const IdMapping& other) {
-    mask_ = other.mask_;
-    entries_ = other.entries_;
-    return *this;
-  }
-
   int operator[](int key) const {
     return get(key);
   }

http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc
index 503ebea..87ba59a 100644
--- a/src/kudu/common/schema-test.cc
+++ b/src/kudu/common/schema-test.cc
@@ -23,6 +23,7 @@
 #include <string>
 #include <tuple>  // IWYU pragma: keep
 #include <unordered_map>
+#include <utility>
 #include <vector>
 
 #include <glog/logging.h> // IWYU pragma: keep
@@ -102,6 +103,65 @@ TEST_F(TestSchema, TestSchema) {
   EXPECT_EQ("uint32 NULLABLE", schema.column(1).TypeToString());
 }
 
+TEST_F(TestSchema, TestCopyAndMove) {
+  auto check_schema = [](const Schema& schema) {
+    ASSERT_EQ(sizeof(Slice) + sizeof(uint32_t) + sizeof(int32_t),
+              schema.byte_size());
+    ASSERT_EQ(3, schema.num_columns());
+    ASSERT_EQ(0, schema.column_offset(0));
+    ASSERT_EQ(sizeof(Slice), schema.column_offset(1));
+
+    EXPECT_EQ("Schema [\n"
+              "\tprimary key (key),\n"
+              "\tkey[string NOT NULL],\n"
+              "\tuint32val[uint32 NULLABLE],\n"
+              "\tint32val[int32 NOT NULL]\n"
+              "]",
+              schema.ToString());
+    EXPECT_EQ("key[string NOT NULL]", schema.column(0).ToString());
+    EXPECT_EQ("uint32 NULLABLE", schema.column(1).TypeToString());
+  };
+
+  ColumnSchema col1("key", STRING);
+  ColumnSchema col2("uint32val", UINT32, true);
+  ColumnSchema col3("int32val", INT32);
+
+  vector<ColumnSchema> cols = { col1, col2, col3 };
+  Schema schema(cols, 1);
+  check_schema(schema);
+
+  // Check copy- and move-assignment.
+  Schema moved_schema;
+  {
+    Schema copied_schema = schema;
+    check_schema(copied_schema);
+    ASSERT_TRUE(copied_schema.Equals(schema));
+
+    // Move-assign to 'moved_to_schema' from 'copied_schema' and then let
+    // 'copied_schema' go out of scope to make sure none of 'moved_to_schema''s
+    // resources are incorrectly freed.
+    moved_schema = std::move(copied_schema);
+
+    // 'copied_schema' is moved from so it should still be valid to call
+    // ToString(), though we can't expect any particular result.
+    NO_FATALS(copied_schema.ToString()); // NOLINT(*)
+  }
+  check_schema(moved_schema);
+  ASSERT_TRUE(moved_schema.Equals(schema));
+
+  // Check copy- and move-construction.
+  {
+    Schema copied_schema(schema);
+    check_schema(copied_schema);
+    ASSERT_TRUE(copied_schema.Equals(schema));
+
+    Schema moved_schema(std::move(copied_schema));
+    NO_FATALS(copied_schema.ToString()); // NOLINT(*)
+    check_schema(moved_schema);
+    ASSERT_TRUE(moved_schema.Equals(schema));
+  }
+}
+
 // Test basic functionality of Schema definition with decimal columns
 TEST_F(TestSchema, TestSchemaWithDecimal) {
   ColumnSchema col1("key", STRING);
@@ -163,21 +223,6 @@ TEST_F(TestSchema, TestSchemaEqualsWithDecimal) {
   EXPECT_FALSE(schema_18_10.Equals(schema_17_9));
 }
 
-TEST_F(TestSchema, TestSwap) {
-  Schema schema1({ ColumnSchema("col1", STRING),
-                   ColumnSchema("col2", STRING),
-                   ColumnSchema("col3", UINT32) },
-                 2);
-  Schema schema2({ ColumnSchema("col3", UINT32),
-                   ColumnSchema("col2", STRING) },
-                 1);
-  schema1.swap(schema2);
-  ASSERT_EQ(2, schema1.num_columns());
-  ASSERT_EQ(1, schema1.num_key_columns());
-  ASSERT_EQ(3, schema2.num_columns());
-  ASSERT_EQ(2, schema2.num_key_columns());
-}
-
 TEST_F(TestSchema, TestColumnSchemaEquals) {
   Slice default_str("read-write default");
   ColumnSchema col1("key", STRING);
@@ -233,11 +278,10 @@ TEST_F(TestSchema, TestReset) {
                          1));
   ASSERT_TRUE(schema.initialized());
 
-  // Swap the initialized schema with an uninitialized one.
+  // Move an uninitialized schema into the initialized schema.
   Schema schema2;
-  schema2.swap(schema);
+  schema = std::move(schema2);
   ASSERT_FALSE(schema.initialized());
-  ASSERT_TRUE(schema2.initialized());
 }
 
 // Test for KUDU-943, a bug where we suspected that Variant didn't behave

http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc
index c999e6a..b15652e 100644
--- a/src/kudu/common/schema.cc
+++ b/src/kudu/common/schema.cc
@@ -141,8 +141,7 @@ size_t ColumnSchema::memory_footprint_including_this() const {
 
 Schema::Schema(const Schema& other)
   : name_to_index_bytes_(0),
-    // TODO(adar): C++11 provides a single-arg constructor
-    name_to_index_(10,
+    name_to_index_(/*bucket_count=*/10,
                    NameToIndexMap::hasher(),
                    NameToIndexMap::key_equal(),
                    NameToIndexMapAllocator(&name_to_index_bytes_)) {
@@ -175,15 +174,45 @@ void Schema::CopyFrom(const Schema& other) {
   has_nullables_ = other.has_nullables_;
 }
 
-void Schema::swap(Schema& other) {
-  std::swap(num_key_columns_, other.num_key_columns_);
-  cols_.swap(other.cols_);
-  col_ids_.swap(other.col_ids_);
-  col_offsets_.swap(other.col_offsets_);
-  name_to_index_.swap(other.name_to_index_);
-  id_to_index_.swap(other.id_to_index_);
-  std::swap(has_nullables_, other.has_nullables_);
+Schema::Schema(Schema&& other) noexcept
+    : cols_(std::move(other.cols_)),
+      num_key_columns_(other.num_key_columns_),
+      col_ids_(std::move(other.col_ids_)),
+      col_offsets_(std::move(other.col_offsets_)),
+      name_to_index_bytes_(0),
+      name_to_index_(/*bucket_count=*/10,
+                     NameToIndexMap::hasher(),
+                     NameToIndexMap::key_equal(),
+                     NameToIndexMapAllocator(&name_to_index_bytes_)),
+      id_to_index_(std::move(other.id_to_index_)),
+      has_nullables_(other.has_nullables_) {
+  // 'name_to_index_' uses a customer allocator which holds a pointer to
+  // 'name_to_index_bytes_'. swap() will swap the contents but not the
+  // allocators; std::move will move the allocator[1], which will mean the moved
+  // to value will be holding a pointer to the moved-from's member, which is
+  // wrong and will likely lead to use-after-free. The 'name_to_index_bytes_'
+  // values should also be swapped so both schemas end up in a valid state.
+  //
+  // [1] STLCountingAllocator::propagate_on_container_move_assignment::value
+  //     is std::true_type, which it inherits from the default Allocator.
   std::swap(name_to_index_bytes_, other.name_to_index_bytes_);
+  name_to_index_.swap(other.name_to_index_);
+}
+
+Schema& Schema::operator=(Schema&& other) noexcept {
+  if (&other != this) {
+    cols_ = std::move(other.cols_);
+    num_key_columns_ = other.num_key_columns_;
+    col_ids_ = std::move(other.col_ids_);
+    col_offsets_ = std::move(other.col_offsets_);
+    id_to_index_ = std::move(other.id_to_index_);
+    has_nullables_ = other.has_nullables_;
+
+    // See the comment in the move constructor implementation for why we swap.
+    std::swap(name_to_index_bytes_, other.name_to_index_bytes_);
+    name_to_index_.swap(other.name_to_index_);
+  }
+  return *this;
 }
 
 Status Schema::Reset(const vector<ColumnSchema>& cols,

http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema.h
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index b5e2003..449f3cd 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -400,10 +400,10 @@ class ColumnSchema {
 // which prefix of columns makes up the primary key.
 //
 // Note that, while Schema is copyable and assignable, it is a complex
-// object that is not inexpensive to copy. You should generally prefer
-// passing by pointer or reference, and functions that create new
+// object that is not inexpensive to copy. Prefer move semantics when
+// possible, or pass by pointer or reference. Functions that create new
 // Schemas should generally prefer taking a Schema pointer and using
-// Schema::swap() or Schema::Reset() rather than returning by value.
+// Schema::Reset() rather than returning by value.
 class Schema {
  public:
 
@@ -412,8 +412,10 @@ class Schema {
   Schema()
     : num_key_columns_(0),
       name_to_index_bytes_(0),
-      // TODO: C++11 provides a single-arg constructor
-      name_to_index_(10,
+      // TODO(wdberkeley): C++11 provides a single-argument constructor, but
+      // it's not supported in GCC < 4.9. This (and the other occurrences here
+      // and in schema.cc) can be fixed if we adopt C++14 or a later standard.
+      name_to_index_(/*bucket_count=*/10,
                      NameToIndexMap::hasher(),
                      NameToIndexMap::key_equal(),
                      NameToIndexMapAllocator(&name_to_index_bytes_)),
@@ -423,9 +425,8 @@ class Schema {
   Schema(const Schema& other);
   Schema& operator=(const Schema& other);
 
-  // TODO(todd) implement a move constructor
-
-  void swap(Schema& other); // NOLINT(build/include_what_you_use)
+  Schema(Schema&& other) noexcept;
+  Schema& operator=(Schema&& other) noexcept;
 
   void CopyFrom(const Schema& other);
 
@@ -438,8 +439,7 @@ class Schema {
   Schema(const std::vector<ColumnSchema>& cols,
          int key_columns)
     : name_to_index_bytes_(0),
-      // TODO: C++11 provides a single-arg constructor
-      name_to_index_(10,
+      name_to_index_(/*bucket_count=*/10,
                      NameToIndexMap::hasher(),
                      NameToIndexMap::key_equal(),
                      NameToIndexMapAllocator(&name_to_index_bytes_)) {
@@ -456,8 +456,7 @@ class Schema {
          const std::vector<ColumnId>& ids,
          int key_columns)
     : name_to_index_bytes_(0),
-      // TODO: C++11 provides a single-arg constructor
-      name_to_index_(10,
+      name_to_index_(/*bucket_count=*/10,
                      NameToIndexMap::hasher(),
                      NameToIndexMap::key_equal(),
                      NameToIndexMapAllocator(&name_to_index_bytes_)) {
@@ -857,7 +856,6 @@ class Schema {
   size_t memory_footprint_including_this() const;
 
  private:
-
   // Return a stringified version of the first 'num_columns' columns of the
   // row.
   template<class RowType>
@@ -906,8 +904,9 @@ class Schema {
   // Cached indicator whether any columns are nullable.
   bool has_nullables_;
 
-  // NOTE: if you add more members, make sure to add the appropriate
-  // code to swap() and CopyFrom() as well to prevent subtle bugs.
+  // NOTE: if you add more members, make sure to add the appropriate code to
+  // CopyFrom() and the move constructor and assignment operator as well, to
+  // prevent subtle bugs.
 };
 
 // Helper used for schema creation/editing.


Mime
View raw message