Fix move constructors to be noexcept

I noticed while looking at a profile of a tablet server starting up that
I saw a lot of CPU used in stacks like:

  kudu::subtle::RefCountedThreadSafeBase::AddRef() const
  void std::vector<scoped_refptr<kudu::fs::internal::LogBlock>>::
       _M_emplace_back_aux<scoped_refptr<kudu::fs::internal::LogBlock>>

Digging into the source of std::vector, I saw that the emplace_back_aux
function is used when emplacing requires resizing the underlying array.
Since scoped_refptr has a move constructor I was surprised to see it
using AddRef() here during the array expansion. In fact it turns out
that std::vector<> will only use the move constructor when it is marked
'noexcept', which ours was not.

I verified this with a simple test program as well[1]. If the move
constructor is marked 'noexcept', it is called. Otherwise, it is not.

This patch corrects scoped_refptr as well as a few other spots where we
had move constructors without noexcept.

[1] https://gist.github.com/toddlipcon/c9a807c6a67037ccd1b63caa93f74c67

Change-Id: Idd7487766be3261e3506ff9613a5d733013018b0
Reviewed-on: http://gerrit.cloudera.org:8080/9382
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <aser...@cloudera.com>


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

Branch: refs/heads/master
Commit: 1cf8ad189719eda84c1a28e7b9dcddfd972c7b98
Parents: ab6296f
Author: Todd Lipcon <t...@apache.org>
Authored: Wed Feb 21 11:56:32 2018 -0800
Committer: Alexey Serbin <aser...@cloudera.com>
Committed: Fri Feb 23 05:36:28 2018 +0000

----------------------------------------------------------------------
 src/kudu/cfile/block_cache.h    | 6 +++---
 src/kudu/cfile/block_handle.h   | 4 ++--
 src/kudu/gutil/ref_counted.h    | 4 ++--
 src/kudu/tablet/lock_manager.cc | 4 ++--
 src/kudu/tablet/lock_manager.h  | 4 ++--
 src/kudu/util/cow_object.h      | 4 ++--
 src/kudu/util/status.h          | 8 ++++----
 7 files changed, 17 insertions(+), 17 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/1cf8ad18/src/kudu/cfile/block_cache.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/block_cache.h b/src/kudu/cfile/block_cache.h
index cdb5520..83f5aff 100644
--- a/src/kudu/cfile/block_cache.h
+++ b/src/kudu/cfile/block_cache.h
@@ -81,7 +81,7 @@ class BlockCache {
     PendingEntry(Cache* cache, Cache::PendingHandle* handle)
         : cache_(cache), handle_(handle) {
     }
-    PendingEntry(PendingEntry&& other) : PendingEntry() {
+    PendingEntry(PendingEntry&& other) noexcept : PendingEntry() {
       *this = std::move(other);
     }
 
@@ -89,7 +89,7 @@ class BlockCache {
       reset();
     }
 
-    PendingEntry& operator=(PendingEntry&& other);
+    PendingEntry& operator=(PendingEntry&& other) noexcept;
     PendingEntry& operator=(const PendingEntry& other) = delete;
 
     // Free the pending entry back to the block cache.
@@ -224,7 +224,7 @@ class BlockCacheHandle {
 
 
 inline BlockCache::PendingEntry& BlockCache::PendingEntry::operator=(
-    BlockCache::PendingEntry&& other) {
+    BlockCache::PendingEntry&& other) noexcept {
   reset();
   cache_ = other.cache_;
   handle_ = other.handle_;

http://git-wip-us.apache.org/repos/asf/kudu/blob/1cf8ad18/src/kudu/cfile/block_handle.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/block_handle.h b/src/kudu/cfile/block_handle.h
index 3a5dbf1..d1798ad 100644
--- a/src/kudu/cfile/block_handle.h
+++ b/src/kudu/cfile/block_handle.h
@@ -43,10 +43,10 @@ class BlockHandle {
     : is_data_owner_(false) { }
 
   // Move constructor and assignment
-  BlockHandle(BlockHandle&& other) {
+  BlockHandle(BlockHandle&& other) noexcept {
     TakeState(&other);
   }
-  BlockHandle& operator=(BlockHandle&& other) {
+  BlockHandle& operator=(BlockHandle&& other) noexcept {
     TakeState(&other);
     return *this;
   }

http://git-wip-us.apache.org/repos/asf/kudu/blob/1cf8ad18/src/kudu/gutil/ref_counted.h
----------------------------------------------------------------------
diff --git a/src/kudu/gutil/ref_counted.h b/src/kudu/gutil/ref_counted.h
index 776b5b8..c3ab8b0 100644
--- a/src/kudu/gutil/ref_counted.h
+++ b/src/kudu/gutil/ref_counted.h
@@ -251,13 +251,13 @@ class scoped_refptr {
 
   // 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()) {
+  scoped_refptr(scoped_refptr&& r) noexcept : ptr_(r.get()) { // NOLINT
     r.ptr_ = nullptr;
   }
 
   // Move conversion constructor.
   template <typename U>
-  scoped_refptr(scoped_refptr<U>&& r) : ptr_(r.get()) {
+  scoped_refptr(scoped_refptr<U>&& r) noexcept : ptr_(r.get()) { // NOLINT
     r.ptr_ = nullptr;
   }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/1cf8ad18/src/kudu/tablet/lock_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/lock_manager.cc b/src/kudu/tablet/lock_manager.cc
index 5b18135..e914994 100644
--- a/src/kudu/tablet/lock_manager.cc
+++ b/src/kudu/tablet/lock_manager.cc
@@ -289,11 +289,11 @@ ScopedRowLock::ScopedRowLock(LockManager *manager,
   }
 }
 
-ScopedRowLock::ScopedRowLock(ScopedRowLock&& other) {
+ScopedRowLock::ScopedRowLock(ScopedRowLock&& other) noexcept {
   TakeState(&other);
 }
 
-ScopedRowLock& ScopedRowLock::operator=(ScopedRowLock&& other) {
+ScopedRowLock& ScopedRowLock::operator=(ScopedRowLock&& other) noexcept {
   TakeState(&other);
   return *this;
 }

http://git-wip-us.apache.org/repos/asf/kudu/blob/1cf8ad18/src/kudu/tablet/lock_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/lock_manager.h b/src/kudu/tablet/lock_manager.h
index 27e9af5..01abd43 100644
--- a/src/kudu/tablet/lock_manager.h
+++ b/src/kudu/tablet/lock_manager.h
@@ -104,8 +104,8 @@ class ScopedRowLock {
                 const Slice &key, LockManager::LockMode mode);
 
   // Move constructor and assignment.
-  ScopedRowLock(ScopedRowLock&& other);
-  ScopedRowLock& operator=(ScopedRowLock&& other);
+  ScopedRowLock(ScopedRowLock&& other) noexcept;
+  ScopedRowLock& operator=(ScopedRowLock&& other) noexcept;
 
   void Release();
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/1cf8ad18/src/kudu/util/cow_object.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/cow_object.h b/src/kudu/util/cow_object.h
index 55c30b0..159a8bb 100644
--- a/src/kudu/util/cow_object.h
+++ b/src/kudu/util/cow_object.h
@@ -205,13 +205,13 @@ class CowLock {
   CowLock& operator=(const CowLock&) = delete;
 
   // Allow moving.
-  CowLock(CowLock&& other)
+  CowLock(CowLock&& other) noexcept
     : cow_(other.cow_),
       mode_(other.mode_) {
     other.cow_ = nullptr;
     other.mode_ = LockMode::RELEASED;
   }
-  CowLock& operator=(CowLock&& other) {
+  CowLock& operator=(CowLock&& other) noexcept {
     cow_ = other.cow_;
     mode_ = other.mode_;
     other.cow_ = nullptr;

http://git-wip-us.apache.org/repos/asf/kudu/blob/1cf8ad18/src/kudu/util/status.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/status.h b/src/kudu/util/status.h
index afcdc8f..3c8a1d9 100644
--- a/src/kudu/util/status.h
+++ b/src/kudu/util/status.h
@@ -167,14 +167,14 @@ class KUDU_EXPORT Status {
   ///
   /// @param [in] s
   ///   rvalue reference to a Status object.
-  Status(Status&& s);
+  Status(Status&& s) noexcept;
 
   /// Assign the specified status using move semantics (C++11).
   ///
   /// @param [in] s
   ///   rvalue reference to a Status object.
   /// @return The reference to the modified object.
-  Status& operator=(Status&& s);
+  Status& operator=(Status&& s) noexcept;
 
   /// If this status is OK, calls 'op' and returns the result, otherwise 
returns
   /// this status.
@@ -474,11 +474,11 @@ inline Status& Status::operator=(const Status& s) {
 }
 
 #if __cplusplus >= 201103L
-inline Status::Status(Status&& s) : state_(s.state_) {
+inline Status::Status(Status&& s) noexcept : state_(s.state_) {
   s.state_ = nullptr;
 }
 
-inline Status& Status::operator=(Status&& s) {
+inline Status& Status::operator=(Status&& s) noexcept {
   if (state_ != s.state_) {
     delete[] state_;
     state_ = s.state_;

Reply via email to