[IMPALA-3161] Impala's gperfutil spinlock suffers from potential wakeup delays

• Type: Bug
• Status: Resolved
• Priority: Critical
• Resolution: Duplicate
• Affects Version/s: Impala 2.5.0
• Component/s: Backend
• Target Version: Impala 2.6.0
• Assignee: Daniel Hecht
• Reporter: Daniel Hecht
• Created: 09/Mar/16 05:58
• Updated: 15/Mar/16 22:53
• Resolved: 15/Mar/16 22:53

Description

While bringing in gutil/spinlock from kudu for IMPALA-3136, I noticed that the spinlock version in Impala's gperfutil (tcmalloc) does not have the below commit. It's plausible this is the cause of IMPALA-3107.

https://github.com/gperftools/gperftools/commit/560ca8650c8a9d2971420970f0ed5e17848150eb

With description:

issue-491: Significant performance improvement for spin lock contention
This patch fixes issues where spinlocks under contention were failing to
wakeup waiters, sometimes resulting in blow ups from 13ns to as high as 256ms.
Under heavy contention, applications were observed sleeping for minutes at a
time giving the appearance of a hang.  

* Author: Sanjay Ghemawat
   */
  
 -//
 -// Fast spinlocks (at least on x86, a lock/unlock pair is approximately
 -// half the cost of a Mutex because the unlock just does a store instead
 -// of a compare-and-swap which is expensive).
 -
  // SpinLock is async signal safe.
  // If used within a signal handler, all lock holders
  // should block the signal even outside the signal handler.
 @@ -95,10 +90,9 @@ class LOCKABLE SpinLock {
    // TODO(csilvers): uncomment the annotation when we figure out how to
    //                 support this macro with 0 args (see thread_annotations.h)
    inline void Unlock() /*UNLOCK_FUNCTION()*/ {
 -    uint64 wait_cycles =
 -        static_cast<uint64>(base::subtle::NoBarrier_Load(&lockword_));
      ANNOTATE_RWLOCK_RELEASED(this, 1);
 -    base::subtle::Release_Store(&lockword_, kSpinLockFree);
 +    uint64 wait_cycles = static_cast<uint64>(
 +        base::subtle::Release_AtomicExchange(&lockword_, kSpinLockFree));
      if (wait_cycles != kSpinLockHeld) {
        // Collect contentionz profile info, and speed the wakeup of any waiter.
        // The wait_cycles value indicates how long this thread spent waiting

Issue Links

• is related to: IMPALA-3162 Upgrade gperfutils