Currently, Windows Threading implementation is largely different. For example, once the thread is detached, we cannot do anything in Windows threading implementation. However, in Pthread Threading implementation, we can take various information and we can do several operations since the thread still remains in ThreadMap. In this bug, we would like to introduce RefPtr<Thread> class instead of ThreadIdentifier. Even if the actual thread is dead, RefPtr<Thread> is valid and we can still get various information (like, thread is already dead). And Thread class should have various useful methods, like suspend and resume. They should be carried from JSC's heap/. And by using RefPtr<Thread>, we can drop ThreadMap. Currently, we always need to look up the platform thread information from the hash map. Instead, RefPtr<Thread> holds this information. And RefPtr<Thread>'s actual thread lifetime is managed by thread exit handler registered by using ThreadSpecific<> thing, which is similar to the current ThreadIdentifierDataPthreads.cpp.
(In reply to Yusuke Suzuki from comment #0) > Currently, Windows Threading implementation is largely different. For > example, once the thread is detached, we cannot do anything in Windows > threading implementation. However, in Pthread Threading implementation, we > can take various information and we can do several operations since the > thread still remains in ThreadMap. > > In this bug, we would like to introduce RefPtr<Thread> class instead of > ThreadIdentifier. Even if the actual thread is dead, RefPtr<Thread> is valid > and we can still get various information (like, thread is already dead). > And Thread class should have various useful methods, like suspend and > resume. They should be carried from JSC's heap/. > > And by using RefPtr<Thread>, we can drop ThreadMap. Currently, we always > need to look up the platform thread information from the hash map. Instead, > RefPtr<Thread> holds this information. And RefPtr<Thread>'s actual thread > lifetime is managed by thread exit handler registered by using > ThreadSpecific<> thing, which is similar to the current > ThreadIdentifierDataPthreads.cpp. And this will drop giant lock for ThreadMap. Each RefPtr<Thread> should have a fine-grained lock to guard it from simultaneous accesses.
Created attachment 306299 [details] WIP WIP: GTK and JSCOnly work on Linux. Only WTF & JSC are changed
Created attachment 306488 [details] WIP WIP
Attachment 306488 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolderPthreads.cpp:35: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WTF/wtf/ThreadingPthreads.cpp:413: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadingPthreads.cpp:436: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WTF/wtf/ThreadingPthreads.cpp:528: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:513: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:33: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WTF/wtf/Threading.h:180: The parameter name "handle" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/Threading.h:180: The parameter name "threadID" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/workers/WorkerThread.cpp:196: 'thread' is incorrectly named. It should be named 'protector' or 'protectedThread'. [readability/naming/protected] [4] Total errors found: 11 in 99 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306490 [details] WIP WIP
Created attachment 306491 [details] WIP WIP
Attachment 306491 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingPthreads.cpp:399: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:513: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 4 in 98 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306493 [details] WIP WIP
Attachment 306493 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingPthreads.cpp:399: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:513: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 4 in 99 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306495 [details] WIP WIP
Attachment 306495 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingPthreads.cpp:399: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:513: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 4 in 97 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306496 [details] WIP WIP
Attachment 306496 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingPthreads.cpp:399: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:513: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 4 in 97 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306499 [details] WIP WIP
Attachment 306499 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingPthreads.cpp:399: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:513: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 4 in 97 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306501 [details] WIP WIP
Attachment 306501 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:245: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 3 in 97 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306506 [details] WIP WIP
Attachment 306506 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:246: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 3 in 96 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306513 [details] WIP WIP
Attachment 306513 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:246: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 3 in 97 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306521 [details] WIP WIP
Attachment 306521 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolderWin.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadingWin.cpp:251: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderPthreads.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 3 in 97 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306617 [details] Patch
Attachment 306617 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingWin.cpp:251: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/PlatformRegisters.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:82: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:82: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:96: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 99 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306619 [details] Patch
Attachment 306619 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingWin.cpp:251: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/PlatformRegisters.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:84: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:98: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 99 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306620 [details] Patch
Attachment 306620 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingWin.cpp:251: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/PlatformRegisters.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:84: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:98: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 99 files If any of these errors are false positives, please file a bug against check-webkit-style.
OK, ready.
Comment on attachment 306620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306620&action=review Looks good so far, but not finished reviewing yet. Here are some comments for now ... > Source/JavaScriptCore/heap/MachineStackMarker.cpp:143 > -Thread* MachineThreads::machineThreadForCurrentThread() > +JSThread* MachineThreads::machineThreadForCurrentThread() This function being named machineThreadForCurrentThread() returning a JSThread* suggests to me that either: 1. JSThread should be renamed MachineThread, or 2. machineThreadForCurrentThread() should be renamed jsThreadForCurrentThread(). I understand that affinity between the JSThread and JS code running on the thread. However, I think MachineThread might be the better name. Here's why: there is one MachineThread for each WTF::Thread that the VM has ever run on, while conceptually, there is only one JS thread running per VM at any one moment. If a VM used to run on MachineThread M1 but now only runs on M2, it still needs to keep track of M1 even though M2 is the real JS thread (thread running JS) now. Hence, MachineThread is the better name because it describes a record of the machine thread that this VM once ran on. My proposal is to go with (1) rename JSThread to MachineThread. Do you agree? > Source/JavaScriptCore/heap/MachineStackMarker.cpp:374 > + "JavaScript garbage collection encountered an invalid thread (err 0x%x): Thread [%d/%d: %p] id %p.", > + result.error(), index, numberOfThreads, thread, reinterpret_cast<void*>(thread->threadID())); You should use %d now instead of %p for thread->threadID() because ThreadIdentifier is a uint32_t. > Source/WTF/wtf/MainThread.cpp:191 > -bool canAccessThreadLocalDataForThread(ThreadIdentifier threadId) > +bool canAccessThreadLocalDataForThread(ThreadIdentifier thread) > { > - return threadId == currentThread(); > + return thread == currentThread(); nit: Why not stick with threadId or just id? Elsewhere, when we expect thread to be a WTF::Thread* and threadId/threadID/id to be a ThreadIdentifier. I think it's best to stay consistent. > Source/WTF/wtf/ThreadingPthreads.cpp:313 > + ASSERT_WITH_MESSAGE(this != ¤t(), "Currently we don't support suspend the current thread itself."); Can you make this a RELEASE_ASSERT_WITH_MESSAGE()? I don't think thread suspension is in any hot path, and it's better to fail an assertion than end up "deadlocking" oneself with self suspension.
Comment on attachment 306620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306620&action=review Thanks. I'll soon upload the patch with the fix. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:143 >> +JSThread* MachineThreads::machineThreadForCurrentThread() > > This function being named machineThreadForCurrentThread() returning a JSThread* suggests to me that either: > 1. JSThread should be renamed MachineThread, or > 2. machineThreadForCurrentThread() should be renamed jsThreadForCurrentThread(). > > I understand that affinity between the JSThread and JS code running on the thread. However, I think MachineThread might be the better name. Here's why: there is one MachineThread for each WTF::Thread that the VM has ever run on, while conceptually, there is only one JS thread running per VM at any one moment. If a VM used to run on MachineThread M1 but now only runs on M2, it still needs to keep track of M1 even though M2 is the real JS thread (thread running JS) now. > > Hence, MachineThread is the better name because it describes a record of the machine thread that this VM once ran on. My proposal is to go with (1) rename JSThread to MachineThread. Do you agree? Agreed. The name "JSThread" is misleading since it actually includes all the threads that run the target VM at least once. I'll rename JSThread to MachineThread. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:374 >> + result.error(), index, numberOfThreads, thread, reinterpret_cast<void*>(thread->threadID())); > > You should use %d now instead of %p for thread->threadID() because ThreadIdentifier is a uint32_t. Right. Maybe, `%u` is better. Fixed. >> Source/WTF/wtf/MainThread.cpp:191 >> + return thread == currentThread(); > > nit: Why not stick with threadId or just id? Elsewhere, when we expect thread to be a WTF::Thread* and threadId/threadID/id to be a ThreadIdentifier. I think it's best to stay consistent. Right. We should keep using threadId here. Fixed. >> Source/WTF/wtf/ThreadingPthreads.cpp:313 >> + ASSERT_WITH_MESSAGE(this != ¤t(), "Currently we don't support suspend the current thread itself."); > > Can you make this a RELEASE_ASSERT_WITH_MESSAGE()? I don't think thread suspension is in any hot path, and it's better to fail an assertion than end up "deadlocking" oneself with self suspension. Right. Fixed.
Created attachment 306785 [details] Patch
Attachment 306785 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingWin.cpp:251: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/PlatformRegisters.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:84: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:98: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 98 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 306785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306785&action=review r=me with outstanding issues resolved. > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:282 > - , m_threadIdentifier(0) > + , m_thread(nullptr) You can skip this. A RefPtr<T> is null by default. > Source/WTF/wtf/ParallelJobsGeneric.h:57 > - : m_threadID(0) > + : m_thread(nullptr) I think you can skip this. A RefPtr<T> is null by default. > Source/WTF/wtf/ParkingLot.cpp:806 > + callback(currentThreadData->thread.get(), currentThreadData->address); Is this correct? currentThreadData->thread.get() returns a WTF::Thread*. The lambda is expecting a WTF::Thread&. How can this work? I see that the EWS bots are all happy. What am I missing? > Source/WTF/wtf/ThreadHolder.cpp:49 > + // Ideally we'd have this as a release assert everywhere, but that would hurt performane. /performane/performance/. > Source/WTF/wtf/ThreadHolder.h:33 > +#ifndef ThreadHolder_h > +#define ThreadHolder_h Please change to use "#pragma once" instead. > Source/WTF/wtf/ThreadHolder.h:82 > +#endif // ThreadHolder_h Please remove. > Source/WTF/wtf/ThreadHolderWin.cpp:63 > + if (holder) I think you should ASSERT(holder != InvalidThreadHolder) here to make it crash earlier. > Source/WTF/wtf/ThreadHolderWin.cpp:84 > + : m_holder(holder) Please fix the indentation here. > Source/WTF/wtf/ThreadHolderWin.cpp:107 > + // Fill the FLS with the non-nullptr value. While FLS destructor won't be called for that, > + // non-nullptr value tells us that we already destructed ThreadHolder. It leads incorrect > + // use of Thread::current() crash. I suggest replacing "It leads incorrect use of Thread::current() crash." with "This allows us to detect incorrect use of Thread::current() after this point because it will crash." > Source/WTF/wtf/Threading.h:81 > + // Returns ThreadIdentifier directly. It is useful if the user only cares about identity > + // of threads and ensure the thread lifetime. While Thread::current() is not safe if it is called > + // from the destructor of the other TLS data, currentID() always returns meaningful thread ID. I'm a bit confused by what you mean by "and ensure the thread lifetime" here. Can you please clarify? > Source/WTF/wtf/Threading.h:115 > + // Internal platform-specific Thread::create implementation. > + static RefPtr<Thread> createInternal(ThreadFunction, void*, const char* threadName); Let's make this private. > Source/WTF/wtf/Threading.h:132 > +#if USE(PTHREADS) && !OS(DARWIN) > + static void signalHandlerSuspendResume(int, siginfo_t*, void* ucontext); > #endif Let's make this private. > Source/WTF/wtf/ThreadingPthreads.cpp:267 > + // The thread has already exited, do nothing. > // The thread hasn't exited yet, so don't clean anything up. Just signal that we've already joined on it so that it will clean up after itself. This comment is a bit confusing. It seems to say that the thread has both "already exited" and "hasn't exited yet". I suggest changing it to say: "If the thread has already exited, then do nothing. If the thread hasn't exited yet, then just signal that we've already joined on it. In both cases, ThreadHolder::destruct() will take care of removing the thread from the threadMap." > Source/WTF/wtf/ThreadingPthreads.cpp:313 > + RELEASE_ASSERT_WITH_MESSAGE(id() != currentThread(), "We do not support suspend the current thread itself."); typo: /suspend/suspending/. > Source/WTF/wtf/ThreadingWin.cpp:165 > void initializeThreading() > { > - static bool isInitialized; > - > - if (isInitialized) > - return; > - > - isInitialized = true; > - > - WTF::double_conversion::initialize(); > - // StringImpl::empty() does not construct its static string in a threadsafe fashion, > - // so ensure it has been initialized from here. > - StringImpl::empty(); > - threadMapMutex(); > - initializeRandomNumberGenerator(); > - wtfThreadData(); > - initializeDates(); > -} > - > -static HashMap<DWORD, HANDLE>& threadMap() > -{ > - static NeverDestroyed<HashMap<DWORD, HANDLE>> map; > - return map; > + static std::once_flag initializeKey; > + std::call_once(initializeKey, [] { > + WTF::double_conversion::initialize(); > + ThreadHolder::initializeOnce(); > + // StringImpl::empty() does not construct its static string in a threadsafe fashion, > + // so ensure it has been initialized from here. > + StringImpl::empty(); > + initializeRandomNumberGenerator(); > + wtfThreadData(); > + initializeDates(); > + }); > } This is identical to the PThread one with the exception of the PThread one needing to do some extra work for !OS(DARWIN). Over the years, I've found that I've occasionally needed to add new things to initialize to initializeThreading(). Note also that most of the things initialized in this list are not specific to thread implementation, but are system wide things that we want to initialize only once. It would be good to only have one copy of it. Can you move this to a common Threading.cpp, and have it call a initializePlatformThreading() function instead that is defined differently for each platform? I think that that would be cleaner. > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:44 > - , m_renderThread(0) > + , m_renderThread(nullptr) This is not needed. > Source/WebCore/Modules/webdatabase/DatabaseThread.h:72 > + RefPtr<Thread> m_thread { nullptr }; The { nullptr } initialization is not needed. > Source/WebCore/page/ResourceUsageThread.h:65 > + RefPtr<Thread> m_thread { nullptr }; The { nullptr } initialization is not needed. > Source/WebCore/page/scrolling/ScrollingThread.cpp:38 > - : m_threadIdentifier(0) > + : m_thread(nullptr) You can skip this. A RefPtr<T> is null by default. > Source/WebCore/platform/audio/HRTFDatabaseLoader.cpp:67 > + : m_databaseLoaderThread(nullptr) Initialization not needed. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:111 > + RefPtr<Thread> m_thread { nullptr }; Initialization not needed. > Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:95 > + RefPtr<Thread> m_workerThread { nullptr }; You don't need the initializer. A RefPtr<T> is null by default. > Source/WebCore/workers/WorkerThread.cpp:102 > - : m_threadID(0) > + : m_thread(nullptr) You can skip this. A RefPtr<T> is null by default. > Source/WebKit/Storage/StorageThread.cpp:43 > - : m_threadID(0) > + : m_thread(nullptr) You can skip this. A RefPtr<T> is null by default. > Tools/TestWebKitAPI/Tests/WTF/ParkingLot.cpp:-54 > - EXPECT_NE(0u, currentThread()); Why is this removed? > Tools/TestWebKitAPI/Tests/WTF/ParkingLot.cpp:182 > + RefPtr<Thread> lastAwoken { nullptr }; Initialization not needed.
Comment on attachment 306785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306785&action=review Thanks! >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:282 >> + , m_thread(nullptr) > > You can skip this. A RefPtr<T> is null by default. Fixed. >> Source/WTF/wtf/ParallelJobsGeneric.h:57 >> + : m_thread(nullptr) > > I think you can skip this. A RefPtr<T> is null by default. Fixed. >> Source/WTF/wtf/ParkingLot.cpp:806 >> + callback(currentThreadData->thread.get(), currentThreadData->address); > > Is this correct? currentThreadData->thread.get() returns a WTF::Thread*. The lambda is expecting a WTF::Thread&. How can this work? I see that the EWS bots are all happy. What am I missing? currentThreadData->thread.get() returns a WTF::Thread& since thread is Ref<Thread>, not RefPtr<Thread>. >> Source/WTF/wtf/ThreadHolder.cpp:49 >> + // Ideally we'd have this as a release assert everywhere, but that would hurt performane. > > /performane/performance/. Oops, thanks. Fixed. >> Source/WTF/wtf/ThreadHolder.h:33 >> +#define ThreadHolder_h > > Please change to use "#pragma once" instead. Changed. >> Source/WTF/wtf/ThreadHolder.h:82 >> +#endif // ThreadHolder_h > > Please remove. Removed. >> Source/WTF/wtf/ThreadHolderWin.cpp:63 >> + if (holder) > > I think you should ASSERT(holder != InvalidThreadHolder) here to make it crash earlier. Yeah, right. Fixed. >> Source/WTF/wtf/ThreadHolderWin.cpp:84 >> + : m_holder(holder) > > Please fix the indentation here. Oops, fixed. >> Source/WTF/wtf/ThreadHolderWin.cpp:107 >> + // use of Thread::current() crash. > > I suggest replacing "It leads incorrect use of Thread::current() crash." with "This allows us to detect incorrect use of Thread::current() after this point because it will crash." Nice, fixed. >> Source/WTF/wtf/Threading.h:81 >> + // from the destructor of the other TLS data, currentID() always returns meaningful thread ID. > > I'm a bit confused by what you mean by "and ensure the thread lifetime" here. Can you please clarify? It means that we cannot know whether the thread corresponding to the thread ID is alive / dead. If we use this, we should know this fact. And while Pthread ThreadIdentifier is monotonically increasing, I suspect that Windows' one is not because Windows implementation just uses GetCurrentThreadId(). But looking the code in WebKit, I guess some code relies on this monotonically increasing characteristics. I think it may be a long-living bug. We should investigate it in a separate patch. Changed the description. >> Source/WTF/wtf/Threading.h:115 >> + static RefPtr<Thread> createInternal(ThreadFunction, void*, const char* threadName); > > Let's make this private. Nice. Fixed. >> Source/WTF/wtf/Threading.h:132 >> #endif > > Let's make this private. Nice. >> Source/WTF/wtf/ThreadingPthreads.cpp:267 >> // The thread hasn't exited yet, so don't clean anything up. Just signal that we've already joined on it so that it will clean up after itself. > > This comment is a bit confusing. It seems to say that the thread has both "already exited" and "hasn't exited yet". I suggest changing it to say: > "If the thread has already exited, then do nothing. > If the thread hasn't exited yet, then just signal that we've already joined on it. > In both cases, ThreadHolder::destruct() will take care of removing the thread from the threadMap." Nice, fixed. >> Source/WTF/wtf/ThreadingPthreads.cpp:313 >> + RELEASE_ASSERT_WITH_MESSAGE(id() != currentThread(), "We do not support suspend the current thread itself."); > > typo: /suspend/suspending/. Fixed. >> Source/WTF/wtf/ThreadingWin.cpp:165 >> } > > This is identical to the PThread one with the exception of the PThread one needing to do some extra work for !OS(DARWIN). Over the years, I've found that I've occasionally needed to add new things to initialize to initializeThreading(). Note also that most of the things initialized in this list are not specific to thread implementation, but are system wide things that we want to initialize only once. It would be good to only have one copy of it. Can you move this to a common Threading.cpp, and have it call a initializePlatformThreading() function instead that is defined differently for each platform? I think that that would be cleaner. Yeah, sounds nice. It decreases the chance to miss to add things in both Pthread / Win initializeThreading. Fixed. >> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:44 >> + , m_renderThread(nullptr) > > This is not needed. Dropped. >> Source/WebCore/Modules/webdatabase/DatabaseThread.h:72 >> + RefPtr<Thread> m_thread { nullptr }; > > The { nullptr } initialization is not needed. Dropped. >> Source/WebCore/page/ResourceUsageThread.h:65 >> + RefPtr<Thread> m_thread { nullptr }; > > The { nullptr } initialization is not needed. Dropped. >> Source/WebCore/page/scrolling/ScrollingThread.cpp:38 >> + : m_thread(nullptr) > > You can skip this. A RefPtr<T> is null by default. Dropped. >> Source/WebCore/platform/audio/HRTFDatabaseLoader.cpp:67 >> + : m_databaseLoaderThread(nullptr) > > Initialization not needed. Dropped. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:111 >> + RefPtr<Thread> m_thread { nullptr }; > > Initialization not needed. Dropped. >> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:95 >> + RefPtr<Thread> m_workerThread { nullptr }; > > You don't need the initializer. A RefPtr<T> is null by default. Dropped. >> Source/WebCore/workers/WorkerThread.cpp:102 >> + : m_thread(nullptr) > > You can skip this. A RefPtr<T> is null by default. Dropped. >> Source/WebKit/Storage/StorageThread.cpp:43 >> + : m_thread(nullptr) > > You can skip this. A RefPtr<T> is null by default. Dropped. >> Tools/TestWebKitAPI/Tests/WTF/ParkingLot.cpp:-54 >> - EXPECT_NE(0u, currentThread()); > > Why is this removed? Oops, added. >> Tools/TestWebKitAPI/Tests/WTF/ParkingLot.cpp:182 >> + RefPtr<Thread> lastAwoken { nullptr }; > > Initialization not needed. Dropped.
Created attachment 306904 [details] Patch for landing
Attachment 306904 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:100: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 98 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306905 [details] Patch for landing
Attachment 306905 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadingWin.cpp:237: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/ThreadHolderWin.cpp:100: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 98 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 306785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306785&action=review > Source/WTF/wtf/ThreadingWin.cpp:305 > + // It is OK because FLSAlloc's callback will be called even before there are some open handles. > + // This easily ensures that all the thread resources are automatically closed. > + if (m_handle) > + CloseHandle(m_handle); One thing I changed a bit is that I moved this CloseHandle part to ~Thread() since m_handle can be used in waitForCompletion even after the thread is already dead. But ideally, this handle can be closed more aggressively (for example, after waitForCompletion, we can drop this handle.). I'll upload the patch for this separately.
Committed r215265: <http://trac.webkit.org/changeset/215265>
r215268
(In reply to Alex Christensen from comment #43) > r215268 Thanks!