[webkit-reviews] review granted: [Bug 170502] [WTF] Introduce Thread class and use RefPtr<Thread> and align Windows Threading implementation semantics to Pthread one : [Attachment 306785] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 11 23:50:13 PDT 2017
Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 170502: [WTF] Introduce Thread class and use RefPtr<Thread> and align
Windows Threading implementation semantics to Pthread one
https://bugs.webkit.org/show_bug.cgi?id=170502
Attachment 306785: Patch
https://bugs.webkit.org/attachment.cgi?id=306785&action=review
--- Comment #35 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 306785
--> https://bugs.webkit.org/attachment.cgi?id=306785
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.
More information about the webkit-reviews
mailing list