[MERGE #5326 @atulkatti] MSFT:17677205 IdleDecommit hang. Back-off from decommit if UI thread wants to enter IdleDecommit again.

Merge pull request #5326 from atulkatti:Bug17677205.IdleDecommitHang.2

We suspect that this may be a problem only under AppVerifier where decommit maybe running slower than usual. The fix should have no impact under normal conditions.

Author: atulkatti

lib/Common/Memory/AllocatorTelemetryStats.h

@@ -11,18 +11,29 @@ struct AllocatorDecommitStats
 {
     Js::Tick lastLeaveDecommitRegion;
     Js::TickDelta maxDeltaBetweenDecommitRegionLeaveAndDecommit;
+
+    // The following values correspond to when we last entered the critical section for Enter/Leave IdleDecommit and
+    // how long we waited to enter the critical section if we had to wait.
+    Js::Tick lastEnterLeaveIdleDecommitTick;
+    Js::TickDelta lastEnterLeaveIdleDecommitCSWaitTime;
+    Js::TickDelta maxEnterLeaveIdleDecommitCSWaitTime;
+    Js::TickDelta totalEnterLeaveIdleDecommitCSWaitTime;
+
     int64 numDecommitCalls;
     int64 numPagesDecommitted;
     int64 numFreePageCount;
 
     AllocatorDecommitStats() :
         lastLeaveDecommitRegion(),
         maxDeltaBetweenDecommitRegionLeaveAndDecommit(0),
+        lastEnterLeaveIdleDecommitTick(),
+        lastEnterLeaveIdleDecommitCSWaitTime(0),
+        maxEnterLeaveIdleDecommitCSWaitTime(0),
+        totalEnterLeaveIdleDecommitCSWaitTime(0),
         numDecommitCalls(0),
         numPagesDecommitted(0),
         numFreePageCount(0)
     {}
-
 };
 
 struct AllocatorSizes

lib/Common/Memory/IdleDecommitPageAllocator.cpp

@@ -44,7 +44,26 @@ IdleDecommitPageAllocator::EnterIdleDecommit()
         return;
     }
 #ifdef IDLE_DECOMMIT_ENABLED
-    cs.Enter();
+    if (!cs.TryEnter())
+    {
+        AutoResetWaitingToEnterIdleDecommitFlag autoResetWaitingToEnterIdleDecommitFlag(this);
+
+#ifdef ENABLE_BASIC_TELEMETRY
+        AllocatorDecommitStats * decommitStats = this->GetDecommitStats();
+        decommitStats->lastEnterLeaveIdleDecommitTick = Js::Tick::Now();
+#endif
+
+        cs.Enter();
+
+#ifdef ENABLE_BASIC_TELEMETRY
+        decommitStats->lastEnterLeaveIdleDecommitCSWaitTime = Js::Tick::Now() - decommitStats->lastEnterLeaveIdleDecommitTick;
+        decommitStats->totalEnterLeaveIdleDecommitCSWaitTime += decommitStats->lastEnterLeaveIdleDecommitCSWaitTime;
+        if (decommitStats->lastEnterLeaveIdleDecommitCSWaitTime > decommitStats->maxEnterLeaveIdleDecommitCSWaitTime)
+        {
+            decommitStats->maxEnterLeaveIdleDecommitCSWaitTime = decommitStats->lastEnterLeaveIdleDecommitCSWaitTime;
+        }
+#endif
+    }
 
     this->isUsed = false;
     this->hadDecommitTimer = hasDecommitTimer;
@@ -80,7 +99,8 @@ IdleDecommitPageAllocator::LeaveIdleDecommit(bool allowTimer)
     Assert(this->maxFreePageCount == maxIdleDecommitFreePageCount);
 
 #ifdef ENABLE_BASIC_TELEMETRY
-    this->GetDecommitStats()->lastLeaveDecommitRegion = Js::Tick::Now();
+    AllocatorDecommitStats * decommitStats = this->GetDecommitStats();
+    decommitStats->lastLeaveDecommitRegion = Js::Tick::Now();
 #endif
 
 #ifdef IDLE_DECOMMIT_ENABLED
@@ -96,7 +116,25 @@ IdleDecommitPageAllocator::LeaveIdleDecommit(bool allowTimer)
 #ifdef IDLE_DECOMMIT_ENABLED
     if (allowTimer)
     {
-        cs.Enter();
+        if (!cs.TryEnter())
+        {
+            AutoResetWaitingToEnterIdleDecommitFlag autoResetWaitingToEnterIdleDecommitFlag(this);
+
+#ifdef ENABLE_BASIC_TELEMETRY
+            decommitStats->lastEnterLeaveIdleDecommitTick = Js::Tick::Now();
+#endif
+
+            cs.Enter();
+
+#ifdef ENABLE_BASIC_TELEMETRY
+            decommitStats->lastEnterLeaveIdleDecommitCSWaitTime = Js::Tick::Now() - decommitStats->lastEnterLeaveIdleDecommitTick;
+            decommitStats->totalEnterLeaveIdleDecommitCSWaitTime += decommitStats->lastEnterLeaveIdleDecommitCSWaitTime;
+            if (decommitStats->lastEnterLeaveIdleDecommitCSWaitTime > decommitStats->maxEnterLeaveIdleDecommitCSWaitTime)
+            {
+                decommitStats->maxEnterLeaveIdleDecommitCSWaitTime = decommitStats->lastEnterLeaveIdleDecommitCSWaitTime;
+            }
+#endif
+        }
 
         PAGE_ALLOC_VERBOSE_TRACE(_u("LeaveIdleDecommit"));
         Assert(maxIdleDecommitFreePageCount != maxNonIdleDecommitFreePageCount);

lib/Common/Memory/IdleDecommitPageAllocator.h

@@ -41,6 +41,23 @@ class IdleDecommitPageAllocator : public PageAllocator
 #endif
 
 private:
+    class AutoResetWaitingToEnterIdleDecommitFlag
+    {
+    public:
+        AutoResetWaitingToEnterIdleDecommitFlag(IdleDecommitPageAllocator * pageAllocator)
+        {
+            this->pageAllocator = pageAllocator;
+            pageAllocator->waitingToEnterIdleDecommit = true;
+        }
+
+        ~AutoResetWaitingToEnterIdleDecommitFlag()
+        {
+            pageAllocator->waitingToEnterIdleDecommit = false;
+        }
+
+    private:
+        IdleDecommitPageAllocator * pageAllocator;
+    };
 
 #ifdef IDLE_DECOMMIT_ENABLED
 #if DBG_DUMP
@@ -83,5 +100,4 @@ class IdleDecommitPageAllocator : public PageAllocator
     }
 #endif
 };
-
 }

lib/Common/Memory/PageAllocator.cpp

@@ -697,6 +697,10 @@ PageAllocatorBase<TVirtualAlloc, TSegment, TPageSegment>::PageAllocatorBase(Allo
 #endif
     minFreePageCount(0),
     isUsed(false),
+    waitingToEnterIdleDecommit(false),
+#if DBG
+    idleDecommitBackOffCount(0),
+#endif
     idleDecommitEnterCount(1),
     isClosed(false),
     stopAllocationOnOutOfMemory(stopAllocationOnOutOfMemory),
@@ -1971,14 +1975,22 @@ PageAllocatorBase<TVirtualAlloc, TSegment, TPageSegment>::DecommitNow(bool all)
             int numZeroPagesFreed = 0;
 
             // There might be queued zero pages.  Drain them first
-
+            bool zeroPageQueueEmpty = false;
             while (true)
             {
                 FreePageEntry * freePageEntry = PopPendingZeroPage();
                 if (freePageEntry == nullptr)
                 {
+                    zeroPageQueueEmpty = true;
                     break;
                 }
+
+                // Back-off from decommit if we are trying to enter IdleDecommit again.
+                if (this->waitingToEnterIdleDecommit)
+                {
+                    break;
+                }
+
                 PAGE_ALLOC_TRACE_AND_STATS_0(_u("Freeing page from zero queue"));
                 TPageSegment * segment = freePageEntry->segment;
                 uint pageCount = freePageEntry->pageCount;
@@ -2021,6 +2033,7 @@ PageAllocatorBase<TVirtualAlloc, TSegment, TPageSegment>::DecommitNow(bool all)
 
             // Take the lock to make sure the recycler thread has finished zeroing out the pages after
             // we drained the queue
+            if(zeroPageQueueEmpty)
             {
                 AutoCriticalSection autoCS(&backgroundPageQueue->backgroundPageQueueCriticalSection);
                 this->hasZeroQueuedPages = false;
@@ -2099,12 +2112,18 @@ PageAllocatorBase<TVirtualAlloc, TSegment, TPageSegment>::DecommitNow(bool all)
 
             }
             pageToDecommit -= pageDecommitted;
+
+            // Back-off from decommit if we are trying to enter IdleDecommit again.
+            if (this->waitingToEnterIdleDecommit)
+            {
+                break;
+            }
         }
     }
 
-    // decommit pages that are empty
-
-    while (pageToDecommit > 0 && !emptySegments.Empty())
+    // decommit pages that are empty.
+    // back-off from decommit if we are trying to enter IdleDecommit again.
+    while (!this->waitingToEnterIdleDecommit && pageToDecommit > 0 && !emptySegments.Empty())
     {
         if (pageToDecommit >= maxAllocPageCount)
         {
@@ -2131,6 +2150,7 @@ PageAllocatorBase<TVirtualAlloc, TSegment, TPageSegment>::DecommitNow(bool all)
         }
     }
 
+    if(!this->waitingToEnterIdleDecommit)
     {
         typename DListBase<TPageSegment>::EditingIterator i(&segments);
 
@@ -2146,23 +2166,34 @@ PageAllocatorBase<TVirtualAlloc, TSegment, TPageSegment>::DecommitNow(bool all)
             i.MoveCurrentTo(&decommitSegments);
             pageToDecommit -= pageDecommitted;
 
+            // Back-off from decommit if we are trying to enter IdleDecommit again.
+            if (this->waitingToEnterIdleDecommit)
+            {
+                break;
+            }
         }
     }
 
-    Assert(pageToDecommit == 0);
-
+    Assert(pageToDecommit == 0 || this->waitingToEnterIdleDecommit);
+#if DBG
+    if (pageToDecommit != 0 && this->waitingToEnterIdleDecommit)
+    {
+        this->idleDecommitBackOffCount++;
+    }
+#endif
 
 #if DBG_DUMP
-    Assert(this->freePageCount == newFreePageCount + decommitCount);
+    Assert(this->freePageCount == newFreePageCount + decommitCount + pageToDecommit);
 #endif
 
-    this->freePageCount = newFreePageCount;
+    // If we had to back-off from decommiting then we may still have some free pages left to decommit.
+    this->freePageCount = newFreePageCount + pageToDecommit;
 
 #ifdef ENABLE_BASIC_TELEMETRY
     if (this->decommitStats != nullptr)
     {
         this->decommitStats->numPagesDecommitted += decommitCount;
-        this->decommitStats->numFreePageCount += newFreePageCount;
+        this->decommitStats->numFreePageCount += newFreePageCount + pageToDecommit;
     }
 #endif
 

lib/Common/Memory/PageAllocator.h

@@ -858,6 +858,15 @@ class PageAllocatorBase: public PageAllocatorBaseCommon
 
     // Idle Decommit
     bool isUsed;
+    // A flag to indicate we are trying to enter IdleDecommit again and back-off from decommit in DecommitNow. This is to prevent
+    // blocking UI thread for too long. We have seen hangs under AppVerifier and believe this may be due to the decommit being slower
+    // under AppVerifier. This shouldn't be a problem otherwise.
+    bool waitingToEnterIdleDecommit;
+
+#if DBG
+    uint idleDecommitBackOffCount;
+#endif
+
     size_t minFreePageCount;
     uint idleDecommitEnterCount;