[impala] 03/04: IMPALA-9840: Fix data race in InternalQueue

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

stakiar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit e389e857479efa56aab59476e07a23221d66a4c7
Author: Bikramjeet Vig <bi...@gmail.com>
AuthorDate: Mon Jun 8 14:54:46 2020 -0700

    IMPALA-9840: Fix data race in InternalQueue
    
    This patch converts the remaining public methods in InternalQueue to
    be thread safe. These methods were being used during multi threaded
    execution and got flagged by thread sanitizer.
    
    Performance:
    Ran single node perf run with TPCH (scale factor 30) on my local to
    make sure there was no perf impact.
    
    +----------+-----------------------+---------+------------+------------+----------------+
    | Workload | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +----------+-----------------------+---------+------------+------------+----------------+
    | TPCH(30) | parquet / none / none | 6.17    | -1.59%     | 4.33       | -0.30%         |
    +----------+-----------------------+---------+------------+------------+----------------+
    
    Change-Id: Ied72c4573e5d23ba744964c3e8a90851d9c6b31c
    Reviewed-on: http://gerrit.cloudera.org:8080/16051
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/util/internal-queue.h | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/be/src/util/internal-queue.h b/be/src/util/internal-queue.h
index ed367d7..85a46e1 100644
--- a/be/src/util/internal-queue.h
+++ b/be/src/util/internal-queue.h
@@ -81,7 +81,7 @@ class InternalQueueBase {
   /// if the queue is empty. This is O(1).
   T* head() const {
     std::lock_guard<LockType> lock(lock_);
-    if (empty()) return nullptr;
+    if (IsEmptyLocked()) return nullptr;
     return reinterpret_cast<T*>(head_);
   }
 
@@ -89,7 +89,7 @@ class InternalQueueBase {
   /// if the queue is empty. This is O(1).
   T* tail() {
     std::lock_guard<LockType> lock(lock_);
-    if (empty()) return nullptr;
+    if (IsEmptyLocked()) return nullptr;
     return reinterpret_cast<T*>(tail_);
   }
 
@@ -133,7 +133,7 @@ class InternalQueueBase {
     Node* result = nullptr;
     {
       std::lock_guard<LockType> lock(lock_);
-      if (empty()) return nullptr;
+      if (IsEmptyLocked()) return nullptr;
       --size_;
       result = head_;
       head_ = head_->next;
@@ -155,7 +155,7 @@ class InternalQueueBase {
     Node* result = nullptr;
     {
       std::lock_guard<LockType> lock(lock_);
-      if (empty()) return nullptr;
+      if (IsEmptyLocked()) return nullptr;
       --size_;
       result = tail_;
       tail_ = tail_->prev;
@@ -223,8 +223,14 @@ class InternalQueueBase {
     head_ = tail_ = nullptr;
   }
 
-  int size() const { return size_; }
-  bool empty() const { return head_ == nullptr; }
+  int size() const {
+    std::lock_guard<LockType> lock(lock_);
+    return SizeLocked();
+  }
+  bool empty() const {
+    std::lock_guard<LockType> lock(lock_);
+    return IsEmptyLocked();
+  }
 
   /// Returns if the target is on the queue. This is O(1) and does not acquire any locks.
   bool Contains(const T* target) const {
@@ -237,7 +243,7 @@ class InternalQueueBase {
     std::lock_guard<LockType> lock(lock_);
     if (head_ == nullptr) {
       if (tail_ != nullptr) return false;
-      if (size() != 0) return false;
+      if (SizeLocked() != 0) return false;
       return true;
     }
 
@@ -254,7 +260,7 @@ class InternalQueueBase {
       }
       current = next;
     }
-    if (num_elements_found != size()) return false;
+    if (num_elements_found != SizeLocked()) return false;
     return true;
   }
 
@@ -286,6 +292,10 @@ class InternalQueueBase {
 
  private:
   friend struct Node;
+
+  inline int SizeLocked() const { return size_; }
+  inline bool IsEmptyLocked() const { return head_ == nullptr; }
+
   mutable LockType lock_;
   Node *head_, *tail_;
   int size_;