[webkit-changes] [225938] trunk/Source

Thu, 14 Dec 2017 15:51:34 -0800

Title: [225938] trunk/Source
Revision
225938
Author
[email protected]
Date
2017-12-14 15:51:05 -0800 (Thu, 14 Dec 2017)

Log 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.

Modified Paths

Diff

Modified: trunk/Source/WTF/ChangeLog (225937 => 225938)


--- trunk/Source/WTF/ChangeLog	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WTF/ChangeLog	2017-12-14 23:51:05 UTC (rev 225938)
@@ -1,3 +1,26 @@
+2017-12-14  Yusuke Suzuki  <[email protected]>
+
+        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  <[email protected]>
 
         JSObjects should have a mask for loading indexed properties

Modified: trunk/Source/WTF/wtf/ParallelJobsGeneric.cpp (225937 => 225938)


--- trunk/Source/WTF/wtf/ParallelJobsGeneric.cpp	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WTF/wtf/ParallelJobsGeneric.cpp	2017-12-14 23:51:05 UTC (rev 225938)
@@ -94,14 +94,14 @@
     }
 
     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();
                 }
 
@@ -109,12 +109,10 @@
             }
         });
     }
+    m_parent = parent;
 
-    if (m_thread)
-        m_parent = parent;
-
     m_mutex.unlock();
-    return m_thread;
+    return true;
 }
 
 void ParallelEnvironment::ThreadPrivate::execute(ThreadFunction threadFunction, void* parameters)

Modified: trunk/Source/WTF/wtf/Threading.cpp (225937 => 225938)


--- trunk/Source/WTF/wtf/Threading.cpp	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WTF/wtf/Threading.cpp	2017-12-14 23:51:05 UTC (rev 225938)
@@ -129,7 +129,7 @@
     function();
 }
 
-RefPtr<Thread> Thread::tryCreate(const char* name, Function<void()>&& entryPoint)
+Ref<Thread> Thread::create(const char* name, Function<void()>&& entryPoint)
 {
     WTF::initializeThreading();
     Ref<Thread> thread = adoptRef(*new Thread());
@@ -142,10 +142,8 @@
     context->ref();
     {
         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;
 
 #if HAVE(STACK_BOUNDS_FOR_NEW_THREAD)
@@ -160,7 +158,7 @@
     }
 
     ASSERT(!thread->stack().isEmpty());
-    return WTFMove(thread);
+    return thread;
 }
 
 static bool shouldRemoveThreadFromThreadGroup()

Modified: trunk/Source/WTF/wtf/Threading.h (225937 => 225938)


--- trunk/Source/WTF/wtf/Threading.h	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WTF/wtf/Threading.h	2017-12-14 23:51:05 UTC (rev 225938)
@@ -86,13 +86,7 @@
 
     // 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.
     static Thread& current();

Modified: trunk/Source/WebCore/ChangeLog (225937 => 225938)


--- trunk/Source/WebCore/ChangeLog	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebCore/ChangeLog	2017-12-14 23:51:05 UTC (rev 225938)
@@ -1,3 +1,17 @@
+2017-12-14  Yusuke Suzuki  <[email protected]>
+
+        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  <[email protected]>
 
         [MSE] Add isValid() check before using trackBuffer.lastEnqueuedPresentationTime

Modified: trunk/Source/WebCore/workers/WorkerThread.cpp (225937 => 225938)


--- trunk/Source/WebCore/workers/WorkerThread.cpp	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebCore/workers/WorkerThread.cpp	2017-12-14 23:51:05 UTC (rev 225938)
@@ -130,21 +130,19 @@
     workerThreads().remove(this);
 }
 
-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.
     LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
 
     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;
 }
 
 void WorkerThread::workerThread()

Modified: trunk/Source/WebCore/workers/WorkerThread.h (225937 => 225938)


--- trunk/Source/WebCore/workers/WorkerThread.h	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebCore/workers/WorkerThread.h	2017-12-14 23:51:05 UTC (rev 225938)
@@ -63,7 +63,7 @@
 public:
     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);
 
     Thread* thread() const { return m_thread.get(); }

Modified: trunk/Source/WebKit/ChangeLog (225937 => 225938)


--- trunk/Source/WebKit/ChangeLog	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebKit/ChangeLog	2017-12-14 23:51:05 UTC (rev 225938)
@@ -1,3 +1,16 @@
+2017-12-14  Yusuke Suzuki  <[email protected]>
+
+        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  <[email protected]>
 
         REGRESSION (r225789): API tests WKProcessPool.InitialWarmedProcessUsed and WebKit.WebsiteDataStoreCustomPaths are failing.

Modified: trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp (225937 => 225938)


--- trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp	2017-12-14 23:51:05 UTC (rev 225938)
@@ -219,13 +219,11 @@
     // Lock here as well as first thing in the thread so the thread doesn't actually commence until the createThread() call
     // 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;
 }

Modified: trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h (225937 => 225938)


--- trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h	2017-12-14 23:51:05 UTC (rev 225938)
@@ -285,7 +285,7 @@
     Ref<IconRecord> getOrCreateIconRecord(const String& iconURL);
     PageURLRecord* getOrCreatePageURLRecord(const String& pageURL);
 
-    bool m_isEnabled {false };
+    bool m_isEnabled { false };
     bool m_privateBrowsingEnabled { false };
 
     mutable Lock m_syncLock;

Modified: trunk/Source/WebKitLegacy/ChangeLog (225937 => 225938)


--- trunk/Source/WebKitLegacy/ChangeLog	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebKitLegacy/ChangeLog	2017-12-14 23:51:05 UTC (rev 225938)
@@ -1,3 +1,14 @@
+2017-12-14  Yusuke Suzuki  <[email protected]>
+
+        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  <[email protected]>
 
         [WTF] Thread::create should have Thread::tryCreate

Modified: trunk/Source/WebKitLegacy/Storage/StorageThread.cpp (225937 => 225938)


--- trunk/Source/WebKitLegacy/Storage/StorageThread.cpp	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebKitLegacy/Storage/StorageThread.cpp	2017-12-14 23:51:05 UTC (rev 225938)
@@ -50,16 +50,15 @@
     ASSERT(!m_thread);
 }
 
-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;
 }
 
 void StorageThread::threadEntryPoint()

Modified: trunk/Source/WebKitLegacy/Storage/StorageThread.h (225937 => 225938)


--- trunk/Source/WebKitLegacy/Storage/StorageThread.h	2017-12-14 23:26:12 UTC (rev 225937)
+++ trunk/Source/WebKitLegacy/Storage/StorageThread.h	2017-12-14 23:51:05 UTC (rev 225938)
@@ -41,7 +41,7 @@
     StorageThread();
     ~StorageThread();
 
-    bool start();
+    void start();
     void terminate();
 
     void dispatch(Function<void ()>&&);

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to