Changeset 225938 in webkit

Timestamp:

Dec 14, 2017, 3:51:05 PM (9 years ago)

Author:

Yusuke Suzuki

Message:

Drop Thread::tryCreate
​https://bugs.webkit.org/show_bug.cgi?id=180808

Reviewed by Darin Adler.

Source/WebCore:

This change reveals that nobody cares the WorkerThread::start's failure.
We should use Thread::create to ensure thread is actually starting.

• workers/WorkerThread.cpp:

(WebCore::WorkerThread::start):

• workers/WorkerThread.h:

Source/WebKit:

We still return bool since IconDatabase::open returns false if it is opened twice.

• UIProcess/API/glib/IconDatabase.cpp:

(WebKit::IconDatabase::open):

• UIProcess/API/glib/IconDatabase.h:

Source/WebKitLegacy:

• Storage/StorageThread.cpp:

(WebCore::StorageThread::start):

• Storage/StorageThread.h:

Source/WTF:

We remove Thread::tryCreate. When thread creation fails, we have no way to keep WebKit working.
Compared to tryMalloc, Thread::create always consumes fixed size of resource. If it fails,
this is not due to arbitrary large user request. It is not reasonable that some thread creations
are handled gracefully while the other thread creations are not.

If we would like to have the limit of number of users' thread creation (like, calling new Worker
so many times), we should have a soft limit instead of relying on system's hard limit.

• wtf/ParallelJobsGeneric.cpp:

(WTF::ParallelEnvironment::ThreadPrivate::tryLockFor):

• wtf/Threading.cpp:

(WTF::Thread::create):
(WTF::Thread::tryCreate): Deleted.

• wtf/Threading.h:

(WTF::Thread::create): Deleted.

Location:

trunk/Source

Files:

• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/ParallelJobsGeneric.cpp (modified) (2 diffs)
• WTF/wtf/Threading.cpp (modified) (3 diffs)
• WTF/wtf/Threading.h (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/workers/WorkerThread.cpp (modified) (2 diffs)
• WebCore/workers/WorkerThread.h (modified) (1 diff)
• WebKit/ChangeLog (modified) (1 diff)
• WebKit/UIProcess/API/glib/IconDatabase.cpp (modified) (1 diff)
• WebKit/UIProcess/API/glib/IconDatabase.h (modified) (1 diff)
• WebKitLegacy/ChangeLog (modified) (1 diff)
• WebKitLegacy/Storage/StorageThread.cpp (modified) (1 diff)
• WebKitLegacy/Storage/StorageThread.h (modified) (1 diff)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2017-12-14  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Drop Thread::tryCreate
+        https://bugs.webkit.org/show_bug.cgi?id=180808
+
+        Reviewed by Darin Adler.
+
+        We remove Thread::tryCreate. When thread creation fails, we have no way to keep WebKit working.
+        Compared to tryMalloc, Thread::create always consumes fixed size of resource. If it fails,
+        this is not due to arbitrary large user request. It is not reasonable that some thread creations
+        are handled gracefully while the other thread creations are not.
+
+        If we would like to have the limit of number of users' thread creation (like, calling `new Worker`
+        so many times), we should have a soft limit instead of relying on system's hard limit.
+
+        * wtf/ParallelJobsGeneric.cpp:
+        (WTF::ParallelEnvironment::ThreadPrivate::tryLockFor):
+        * wtf/Threading.cpp:
+        (WTF::Thread::create):
+        (WTF::Thread::tryCreate): Deleted.
+        * wtf/Threading.h:
+        (WTF::Thread::create): Deleted.
+
 2017-12-13  Keith Miller  <keith_miller@apple.com>
 

trunk/Source/WTF/wtf/ParallelJobsGeneric.cpp

@@ -95 +95 @@
 
     if (!m_thread) {
-        m_thread = Thread::tryCreate("Parallel worker", [this] {
+        m_thread = Thread::create("Parallel worker", [this] {
             LockHolder lock(m_mutex);
 
-            while (m_thread) {
+            while (true) {
                 if (m_running) {
                     (*m_threadFunction)(m_parameters);
                     m_running = false;
-                    m_parent = 0;
+                    m_parent = nullptr;
                     m_threadCondition.notifyOne();
                 }
@@ -110 +110 @@
         });
     }
-
-    if (m_thread)
-        m_parent = parent;
+    m_parent = parent;
 
     m_mutex.unlock();
-    return m_thread;
+    return true;
 }
 

trunk/Source/WTF/wtf/Threading.cpp

@@ -130 +130 @@
 }
 
-RefPtr<Thread> Thread::tryCreate(const char* name, Function<void()>&& entryPoint)
+Ref<Thread> Thread::create(const char* name, Function<void()>&& entryPoint)
 {
     WTF::initializeThreading();
@@ -143 +143 @@
     {
         MutexLocker locker(context->mutex);
-        if (!thread->establishHandle(context.ptr())) {
-            context->deref();
-            return nullptr;
-        }
+        bool success = thread->establishHandle(context.ptr());
+        RELEASE_ASSERT(success);
         context->stage = NewThreadContext::Stage::EstablishedHandle;
 
@@ -161 +159 @@
 
     ASSERT(!thread->stack().isEmpty());
-    return WTFMove(thread);
+    return thread;
 }
 

trunk/Source/WTF/wtf/Threading.h

@@ -87 +87 @@
     // Returns nullptr if thread creation failed.
     // The thread name must be a literal since on some platforms it's passed in to the thread.
-    WTF_EXPORT_PRIVATE static RefPtr<Thread> tryCreate(const char* threadName, Function<void()>&&);
-    static inline Ref<Thread> create(const char* threadName, Function<void()>&& function)
-    {
-        auto thread = tryCreate(threadName, WTFMove(function));
-        RELEASE_ASSERT(thread);
-        return thread.releaseNonNull();
-    }
+    WTF_EXPORT_PRIVATE static Ref<Thread> create(const char* threadName, Function<void()>&&);
 
     // Returns Thread object.

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-12-14  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Drop Thread::tryCreate
+        https://bugs.webkit.org/show_bug.cgi?id=180808
+
+        Reviewed by Darin Adler.
+
+        This change reveals that nobody cares the WorkerThread::start's failure.
+        We should use `Thread::create` to ensure thread is actually starting.
+
+        * workers/WorkerThread.cpp:
+        (WebCore::WorkerThread::start):
+        * workers/WorkerThread.h:
+
 2017-12-14  Alicia Boya García  <aboya@igalia.com>
 

trunk/Source/WebCore/workers/WorkerThread.cpp

@@ -131 +131 @@
 }
 
-bool WorkerThread::start(WTF::Function<void(const String&)>&& evaluateCallback)
+void WorkerThread::start(WTF::Function<void(const String&)>&& evaluateCallback)
 {
     // Mutex protection is necessary to ensure that m_thread is initialized when the thread starts.
@@ -137 +137 @@
 
     if (m_thread)
-        return true;
+        return;
 
     m_evaluateCallback = WTFMove(evaluateCallback);
 
-    m_thread = Thread::tryCreate("WebCore: Worker", [this] {
+    m_thread = Thread::create("WebCore: Worker", [this] {
         workerThread();
     });
-
-    return m_thread;
 }
 

trunk/Source/WebCore/workers/WorkerThread.h

@@ -64 +64 @@
     virtual ~WorkerThread();
 
-    WEBCORE_EXPORT bool start(WTF::Function<void(const String&)>&& evaluateCallback);
+    WEBCORE_EXPORT void start(WTF::Function<void(const String&)>&& evaluateCallback);
     void stop(WTF::Function<void()>&& terminatedCallback);
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2017-12-14  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Drop Thread::tryCreate
+        https://bugs.webkit.org/show_bug.cgi?id=180808
+
+        Reviewed by Darin Adler.
+
+        We still return bool since IconDatabase::open returns `false` if it is opened twice.
+
+        * UIProcess/API/glib/IconDatabase.cpp:
+        (WebKit::IconDatabase::open):
+        * UIProcess/API/glib/IconDatabase.h:
+
 2017-12-14  Brady Eidson  <beidson@apple.com>
 

trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp

@@ -220 +220 @@
     // completes and m_syncThreadRunning is properly set
     m_syncLock.lock();
-    m_syncThread = Thread::tryCreate("WebCore: IconDatabase", [this] {
+    m_syncThread = Thread::create("WebCore: IconDatabase", [this] {
         iconDatabaseSyncThread();
     });
-    m_syncThreadRunning = m_syncThread;
+    m_syncThreadRunning = true;
     m_syncLock.unlock();
-    if (!m_syncThread)
-        return false;
     return true;
 }

trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h

@@ -286 +286 @@
     PageURLRecord* getOrCreatePageURLRecord(const String& pageURL);
 
-    bool m_isEnabled {false };
+    bool m_isEnabled { false };
     bool m_privateBrowsingEnabled { false };
 

trunk/Source/WebKitLegacy/ChangeLog

@@ +1 @@
+2017-12-14  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Drop Thread::tryCreate
+        https://bugs.webkit.org/show_bug.cgi?id=180808
+
+        Reviewed by Darin Adler.
+
+        * Storage/StorageThread.cpp:
+        (WebCore::StorageThread::start):
+        * Storage/StorageThread.h:
+
 2017-12-12  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/Source/WebKitLegacy/Storage/StorageThread.cpp

@@ -51 +51 @@
 }
 
-bool StorageThread::start()
+void StorageThread::start()
 {
     ASSERT(isMainThread());
     if (!m_thread) {
-        m_thread = Thread::tryCreate("WebCore: LocalStorage", [this] {
+        m_thread = Thread::create("WebCore: LocalStorage", [this] {
             threadEntryPoint();
         });
     }
     activeStorageThreads().add(this);
-    return m_thread;
 }
 

trunk/Source/WebKitLegacy/Storage/StorageThread.h

@@ -42 +42 @@
     ~StorageThread();
 
-    bool start();
+    void start();
     void terminate();