kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject kudu git commit: ref_counted: fix move constructors to actually move
Date Thu, 14 Sep 2017 22:10:14 GMT
Repository: kudu
Updated Branches:
  refs/heads/master fa8620cc7 -> 2c5f24b05


ref_counted: fix move constructors to actually move

Ever since the move constructors for scoped_refptr were first implemented, they
didn't properly move. That is to say, they caused an unnecessary AddRef/Release
pair.

The issue is that they were passing through an rvalue-reference parameter without
properly using std::move(). See [1] for an explanation of this subtlety.

When combined with some other in-flight patches, this resulted in a substantial
improvement in LBM startup time, since a hashmap containing ref-counted elements
could relocate them without unnecessary reference count increments and decrements.
It's likely it will also improve performance elsewhere throughout Kudu.

This patch also pulls in an extra move constructor from the Chromium code base
which they claim helps Clang generate better warnings. I didn't verify that, but
figured I'd copy it while I was looking at the code.

[1] https://stackoverflow.com/questions/14486367/why-do-you-use-stdmove-when-you-have-in-c11

Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6
Reviewed-on: http://gerrit.cloudera.org:8080/8002
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <adar@cloudera.com>
Reviewed-by: Dan Burkert <danburkert@apache.org>


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

Branch: refs/heads/master
Commit: 2c5f24b05c2823bd764a82f3dcb63c9af3edf135
Parents: fa8620c
Author: Todd Lipcon <todd@apache.org>
Authored: Thu Sep 7 12:43:32 2017 -0700
Committer: Todd Lipcon <todd@apache.org>
Committed: Thu Sep 14 22:08:07 2017 +0000

----------------------------------------------------------------------
 src/kudu/gutil/ref_counted.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/2c5f24b0/src/kudu/gutil/ref_counted.h
----------------------------------------------------------------------
diff --git a/src/kudu/gutil/ref_counted.h b/src/kudu/gutil/ref_counted.h
index 66cdac9..0fef5a6 100644
--- a/src/kudu/gutil/ref_counted.h
+++ b/src/kudu/gutil/ref_counted.h
@@ -7,6 +7,7 @@
 
 #include <cassert>
 #include <cstddef>
+#include <utility>
 
 #include "kudu/gutil/atomicops.h"
 #include "kudu/gutil/macros.h"
@@ -235,17 +236,26 @@ class scoped_refptr {
       ptr_->AddRef();
   }
 
+  // Copy constructor.
   scoped_refptr(const scoped_refptr<T>& r) : ptr_(r.ptr_) {
     if (ptr_)
       ptr_->AddRef();
   }
 
+  // Copy conversion constructor.
   template <typename U>
   scoped_refptr(const scoped_refptr<U>& r) : ptr_(r.get()) {
     if (ptr_)
       ptr_->AddRef();
   }
 
+  // Move constructor. This is required in addition to the conversion
+  // constructor below in order for clang to warn about pessimizing moves.
+  scoped_refptr(scoped_refptr&& r) : ptr_(r.get()) {
+    r.ptr_ = nullptr;
+  }
+
+  // Move conversion constructor.
   template <typename U>
   scoped_refptr(scoped_refptr<U>&& r) : ptr_(r.get()) {
     r.ptr_ = nullptr;
@@ -296,13 +306,13 @@ class scoped_refptr {
   }
 
   scoped_refptr<T>& operator=(scoped_refptr<T>&& r) {
-    scoped_refptr<T>(r).swap(*this);
+    scoped_refptr<T>(std::move(r)).swap(*this);
     return *this;
   }
 
   template <typename U>
   scoped_refptr<T>& operator=(scoped_refptr<U>&& r) {
-    scoped_refptr<T>(r).swap(*this);
+    scoped_refptr<T>(std::move(r)).swap(*this);
     return *this;
   }
 


Mime
View raw message