RunLoop::dispatch should only call wakeUp when needed

https://bugs.webkit.org/show_bug.cgi?id=213705

Reviewed by Brady Eidson.

Source/WTF:

RunLoop::wakeUp is super expensive (at least on Darwin platforms). Back
when IndexedDB used RunLoop::dispatch, RunLoop::wakeUp accounted for 15%
of all database lookup time.

We can reduce the cost a bit by only scheduling a wakeUp when the queue
is empty.

* wtf/RunLoop.cpp:
(WTF::RunLoop::performWork): Take all the functions out of the pending
queue before we start executing them; that way, we can tell if a new
function has been added to the pending queue since we enetered
performWork.

(WTF::RunLoop::dispatch): Only call wakeUp if we need to.

* wtf/RunLoop.h:

Tools:

Added some tests for interesting edge cases.

Now that we have tests, we don't need so many comments.

* TestWebKitAPI/Tests/WTF/RunLoop.cpp:
(TestWebKitAPI::TEST): Test that re-entry maintiains ordering. Test that
re-entry does not deadlock with cross-thread posting. (This deadlock is
why we can't just use a naive check for an empty queue.)

* TestWebKitAPI/glib/UtilitiesGLib.cpp:
(TestWebKitAPI::Util::run):
(TestWebKitAPI::Util::spinRunLoop):
(TestWebKitAPI::Util::sleep): Try to fix the GLib RunLoop testing code.
GLib fails my two new tests, and seems to fail other RunLoop-y tests.
I think this is because g_idle_add is defined not to work in nested
RunLoops. Let's see if using WTF's RunLoop API instead can help.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@263973 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Author: [email protected]

Source/WTF/ChangeLog

@@ -1,3 +1,27 @@
+2020-07-06  Geoffrey Garen  <[email protected]>
+
+        RunLoop::dispatch should only call wakeUp when needed
+        https://bugs.webkit.org/show_bug.cgi?id=213705
+
+        Reviewed by Brady Eidson.
+
+        RunLoop::wakeUp is super expensive (at least on Darwin platforms). Back
+        when IndexedDB used RunLoop::dispatch, RunLoop::wakeUp accounted for 15%
+        of all database lookup time.
+
+        We can reduce the cost a bit by only scheduling a wakeUp when the queue
+        is empty.
+
+        * wtf/RunLoop.cpp:
+        (WTF::RunLoop::performWork): Take all the functions out of the pending
+        queue before we start executing them; that way, we can tell if a new
+        function has been added to the pending queue since we enetered
+        performWork.
+
+        (WTF::RunLoop::dispatch): Only call wakeUp if we need to.
+
+        * wtf/RunLoop.h:
+
 2020-07-04  Darin Adler  <[email protected]>
 
         [Cocoa] Add almost everything from FeatureDefines.xcconfig to PlatformEnableCocoa.h

Source/WTF/wtf/RunLoop.cpp

@@ -97,45 +97,25 @@ bool RunLoop::isMain()
 
 void RunLoop::performWork()
 {
-    // It is important to handle the functions in the queue one at a time because while inside one of these
-    // functions we might re-enter RunLoop::performWork() and we need to be able to pick up where we left off.
-    // See http://webkit.org/b/89590 for more discussion.
-
-    // One possible scenario when handling the function queue is as follows:
-    // - RunLoop::performWork() is invoked with 1 function on the queue
-    // - Handling that function results in 1 more function being enqueued
-    // - Handling that one results in yet another being enqueued
-    // - And so on
-    //
-    // In this situation one invocation of performWork() never returns so all other event sources are blocked.
-    // By only handling up to the number of functions that were in the queue when performWork() is called
-    // we guarantee to occasionally return from the run loop so other event sources will be allowed to spin.
-
-    size_t functionsToHandle = 1;
     bool didSuspendFunctions = false;
 
-    for (size_t functionsHandled = 0; functionsHandled < functionsToHandle; ++functionsHandled) {
-        Function<void ()> function;
-        {
-            auto locker = holdLock(m_functionQueueLock);
-
-            // Even if we start off with N functions to handle and we've only handled less than N functions, the queue
-            // still might be empty because those functions might have been handled in an inner RunLoop::performWork().
-            // In that case we should bail here.
-            if (m_functionQueue.isEmpty())
-                break;
+    {
+        auto locker = holdLock(m_nextIterationLock);
 
-            if (m_isFunctionDispatchSuspended) {
-                didSuspendFunctions = true;
-                break;
-            }
+        // If the RunLoop re-enters or re-schedules, we're expected to execute all functions in order.
+        while (!m_currentIteration.isEmpty())
+            m_nextIteration.prepend(m_currentIteration.takeLast());
 
-            if (!functionsHandled)
-                functionsToHandle = m_functionQueue.size();
+        m_currentIteration = std::exchange(m_nextIteration, { });
+    }
 
-            function = m_functionQueue.takeFirst();
+    while (!m_currentIteration.isEmpty()) {
+        if (m_isFunctionDispatchSuspended) {
+            didSuspendFunctions = true;
+            break;
         }
-        
+
+        auto function = m_currentIteration.takeFirst();
         function();
     }
 
@@ -149,12 +129,16 @@ void RunLoop::performWork()
 
 void RunLoop::dispatch(Function<void ()>&& function)
 {
+    bool needsWakeup = false;
+
     {
-        auto locker = holdLock(m_functionQueueLock);
-        m_functionQueue.append(WTFMove(function));
+        auto locker = holdLock(m_nextIterationLock);
+        needsWakeup = m_nextIteration.isEmpty();
+        m_nextIteration.append(WTFMove(function));
     }
 
-    wakeUp();
+    if (needsWakeup)
+        wakeUp();
 }
 
 void RunLoop::suspendFunctionDispatchForCurrentCycle()

Source/WTF/wtf/RunLoop.h

@@ -208,8 +208,11 @@ class RunLoop final : public FunctionDispatcher {
 
     void performWork();
 
-    Lock m_functionQueueLock;
-    Deque<Function<void()>> m_functionQueue;
+    Deque<Function<void()>> m_currentIteration;
+
+    Lock m_nextIterationLock;
+    Deque<Function<void()>> m_nextIteration;
+
     bool m_isFunctionDispatchSuspended { false };
     bool m_hasSuspendedFunctions { false };
 

Tools/ChangeLog

@@ -1,3 +1,27 @@
+2020-07-06  Geoffrey Garen  <[email protected]>
+
+        RunLoop::dispatch should only call wakeUp when needed
+        https://bugs.webkit.org/show_bug.cgi?id=213705
+
+        Reviewed by Brady Eidson.
+
+        Added some tests for interesting edge cases.
+
+        Now that we have tests, we don't need so many comments.
+
+        * TestWebKitAPI/Tests/WTF/RunLoop.cpp:
+        (TestWebKitAPI::TEST): Test that re-entry maintiains ordering. Test that
+        re-entry does not deadlock with cross-thread posting. (This deadlock is
+        why we can't just use a naive check for an empty queue.)
+
+        * TestWebKitAPI/glib/UtilitiesGLib.cpp:
+        (TestWebKitAPI::Util::run):
+        (TestWebKitAPI::Util::spinRunLoop):
+        (TestWebKitAPI::Util::sleep): Try to fix the GLib RunLoop testing code.
+        GLib fails my two new tests, and seems to fail other RunLoop-y tests.
+        I think this is because g_idle_add is defined not to work in nested
+        RunLoops. Let's see if using WTF's RunLoop API instead can help.
+
 2020-07-06  Carlos Garcia Campos  <[email protected]>
 
         [GTK][WPE] Enable storageAccess tests

Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp

@@ -55,6 +55,48 @@ TEST(WTF_RunLoop, Deadlock)
     Util::run(&testFinished);
 }
 
+TEST(WTF_RunLoop, NestedInOrder)
+{
+    WTF::initializeMainThread();
+
+    bool done = false;
+    bool didExecuteOuter = false;
+
+    RunLoop::main().dispatch([&done, &didExecuteOuter] {
+        RunLoop::main().dispatch([&done, &didExecuteOuter] {
+            EXPECT_TRUE(didExecuteOuter);
+            done = true;
+        });
+
+        Util::run(&done);
+    });
+    RunLoop::main().dispatch([&didExecuteOuter] { 
+        didExecuteOuter = true;
+    });
+
+    Util::run(&done);
+}
+
+TEST(WTF_RunLoop, DispatchCrossThreadWhileNested)
+{
+    WTF::initializeMainThread();
+
+    bool done = false;
+
+    RunLoop::main().dispatch([&done] {
+        Thread::create("DispatchCrossThread", [&done] {
+            RunLoop::main().dispatch([&done] {
+                done = true;
+            });
+        });
+
+        Util::run(&done);
+    });
+    RunLoop::main().dispatch([] { });
+
+    Util::run(&done);
+}
+
 class DerivedOneShotTimer : public RunLoop::Timer<DerivedOneShotTimer> {
 public:
     DerivedOneShotTimer(bool& testFinished)

Tools/TestWebKitAPI/glib/UtilitiesGLib.cpp

@@ -34,31 +34,22 @@ namespace Util {
 
 void run(bool* done)
 {
-    g_idle_add([](gpointer userData) -> gboolean {
-        bool* done = static_cast<bool*>(userData);
-        if (*done)
-            RunLoop::current().stop();
-
-        return !*done;
-    }, done);
-    RunLoop::run();
+    while (!*done)
+        RunLoop::current().cycle();
 }
 
 void spinRunLoop(uint64_t count)
 {
-    g_idle_add([](gpointer userData) -> gboolean {
-        uint64_t* count = static_cast<uint64_t*>(userData);
-        return --(*count) ? G_SOURCE_CONTINUE : G_SOURCE_REMOVE;
-    }, &count);
+    while (count--)
+        RunLoop::current().cycle();
 }
 
 void sleep(double seconds)
 {
-    g_timeout_add(seconds * 1000, [](gpointer userData) -> gboolean {
-        RunLoop::main().stop();
-        return G_SOURCE_REMOVE;
-    }, nullptr);
-    RunLoop::current().stop();
+    RunLoop::current().dispatchAfter(Seconds(seconds), [] {
+        RunLoop::current().stop();
+    });
+    RunLoop::current().run();
 }
 
 } // namespace Util