- Revision
- 220548
- Author
- utatane....@gmail.com
- Date
- 2017-08-10 14:19:51 -0700 (Thu, 10 Aug 2017)
Log Message
[WTF] ThreadSpecific should not introduce additional indirection
https://bugs.webkit.org/show_bug.cgi?id=175187
Reviewed by Mark Lam.
Source/_javascript_Core:
* runtime/Identifier.cpp:
Source/WebCore:
We drop ThreadSpecific::replace feature which is only used by
Web thread. Instead, we use ThreadSpecific<std::unique_ptr<T>> here.
While this std::unique_ptr<T> shares one instance between main thread
and web thread, this is the same to the current implementation. It is
safe because the web thread never finishes.
And for non-web thread implementation, we just use ThreadSpecific<T>,
since it is the most efficient.
* platform/ThreadGlobalData.cpp:
(WebCore::ThreadGlobalData::ThreadGlobalData):
(WebCore::ThreadGlobalData::setWebCoreThreadData):
(WebCore::threadGlobalData):
We also drop StringImpl::empty() call since it is not necessary now:
StringImpl::empty data is statically initialized by using constexpr.
* platform/ThreadGlobalData.h:
We make it FAST_ALLOCATED since it is previously allocated by fast malloc
in ThreadSpecific.
Source/WTF:
ThreadSpecific sets Data* to the TLS. And Data holds T*, which
is fast allocated actual data. But ideally, we should store T
instance directly in Data instead of introducing an additional
indirection.
This patch adds storage in Data in order to embed the instance of T. The constructor
for Data will invoke the constructor for T on the embedded storage. We also drop
ThreadSpecific::replace which is only used by the web thread to set its thread specific
ThreadGlobalData to the one shared from the main thread. The existing implementation
relies on the main thread and the web thread never exiting in order for the shared
ThreadGlobalData to stay alive. We can achieve the same semantics by using a
ThreadSpecific<std::unique_ptr<T>> to hold the ThreadGlobalData instance instead.
* wtf/ThreadSpecific.h:
(WTF::ThreadSpecific::Data::construct):
(WTF::ThreadSpecific::Data::Data):
We make it fast allocated since we previously allocated ThreadSpecific T data by fastMalloc.
(WTF::ThreadSpecific::Data::~Data):
(WTF::ThreadSpecific::Data::storagePointer const):
(WTF::canBeGCThread>::get):
We also drop RELEASE_ASSERT from ::get(). We already inserted this assert to setAndConstruct(),
so when creating the member to this TLS, we execute this release assert. So it is
not necessary to execute this assertion every time we get data from this TLS.
(WTF::canBeGCThread>::set):
(WTF::canBeGCThread>::destroy):
(WTF::canBeGCThread>::setAndConstruct):
(WTF::T):
(WTF::canBeGCThread>::replace): Deleted.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (220547 => 220548)
--- trunk/Source/_javascript_Core/ChangeLog 2017-08-10 21:06:04 UTC (rev 220547)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-08-10 21:19:51 UTC (rev 220548)
@@ -1,3 +1,12 @@
+2017-08-09 Yusuke Suzuki <utatane....@gmail.com>
+
+ [WTF] ThreadSpecific should not introduce additional indirection
+ https://bugs.webkit.org/show_bug.cgi?id=175187
+
+ Reviewed by Mark Lam.
+
+ * runtime/Identifier.cpp:
+
2017-08-10 Tim Horton <timothy_hor...@apple.com>
Remove some unused lambda captures so that WebKit builds with -Wunused-lambda-capture
Modified: trunk/Source/_javascript_Core/runtime/Identifier.cpp (220547 => 220548)
--- trunk/Source/_javascript_Core/runtime/Identifier.cpp 2017-08-10 21:06:04 UTC (rev 220547)
+++ trunk/Source/_javascript_Core/runtime/Identifier.cpp 2017-08-10 21:19:51 UTC (rev 220548)
@@ -33,8 +33,6 @@
#include <wtf/text/ASCIIFastPath.h>
#include <wtf/text/StringHash.h>
-using WTF::ThreadSpecific;
-
namespace JSC {
Ref<StringImpl> Identifier::add(VM* vm, const char* c)
Modified: trunk/Source/WTF/ChangeLog (220547 => 220548)
--- trunk/Source/WTF/ChangeLog 2017-08-10 21:06:04 UTC (rev 220547)
+++ trunk/Source/WTF/ChangeLog 2017-08-10 21:19:51 UTC (rev 220548)
@@ -1,3 +1,41 @@
+2017-08-09 Yusuke Suzuki <utatane....@gmail.com>
+
+ [WTF] ThreadSpecific should not introduce additional indirection
+ https://bugs.webkit.org/show_bug.cgi?id=175187
+
+ Reviewed by Mark Lam.
+
+ ThreadSpecific sets Data* to the TLS. And Data holds T*, which
+ is fast allocated actual data. But ideally, we should store T
+ instance directly in Data instead of introducing an additional
+ indirection.
+
+ This patch adds storage in Data in order to embed the instance of T. The constructor
+ for Data will invoke the constructor for T on the embedded storage. We also drop
+ ThreadSpecific::replace which is only used by the web thread to set its thread specific
+ ThreadGlobalData to the one shared from the main thread. The existing implementation
+ relies on the main thread and the web thread never exiting in order for the shared
+ ThreadGlobalData to stay alive. We can achieve the same semantics by using a
+ ThreadSpecific<std::unique_ptr<T>> to hold the ThreadGlobalData instance instead.
+
+ * wtf/ThreadSpecific.h:
+ (WTF::ThreadSpecific::Data::construct):
+ (WTF::ThreadSpecific::Data::Data):
+ We make it fast allocated since we previously allocated ThreadSpecific T data by fastMalloc.
+
+ (WTF::ThreadSpecific::Data::~Data):
+ (WTF::ThreadSpecific::Data::storagePointer const):
+ (WTF::canBeGCThread>::get):
+ We also drop RELEASE_ASSERT from ::get(). We already inserted this assert to setAndConstruct(),
+ so when creating the member to this TLS, we execute this release assert. So it is
+ not necessary to execute this assertion every time we get data from this TLS.
+
+ (WTF::canBeGCThread>::set):
+ (WTF::canBeGCThread>::destroy):
+ (WTF::canBeGCThread>::setAndConstruct):
+ (WTF::T):
+ (WTF::canBeGCThread>::replace): Deleted.
+
2017-08-10 Tim Horton <timothy_hor...@apple.com>
Fix a silly typo in Compiler.h
Modified: trunk/Source/WTF/wtf/ThreadSpecific.h (220547 => 220548)
--- trunk/Source/WTF/wtf/ThreadSpecific.h 2017-08-10 21:06:04 UTC (rev 220547)
+++ trunk/Source/WTF/wtf/ThreadSpecific.h 2017-08-10 21:19:51 UTC (rev 220548)
@@ -82,10 +82,6 @@
operator T*();
T& operator*();
-#if USE(WEB_THREAD)
- void replace(T*);
-#endif
-
private:
// Not implemented. It's technically possible to destroy a thread specific key, but one would need
// to make sure that all values have been destroyed already (usually, that all threads that used it
@@ -93,19 +89,38 @@
// a destructor defined can be confusing, given that it has such strong pre-requisites to work correctly.
~ThreadSpecific();
- T* get();
- void set(T*);
- void static THREAD_SPECIFIC_CALL destroy(void* ptr);
-
struct Data {
WTF_MAKE_NONCOPYABLE(Data);
+ WTF_MAKE_FAST_ALLOCATED;
public:
- Data(T* value, ThreadSpecific<T, canBeGCThread>* owner) : value(value), owner(owner) {}
+ using PointerType = typename std::remove_const<T>::type*;
- T* value;
+ Data(ThreadSpecific<T, canBeGCThread>* owner)
+ : owner(owner)
+ {
+ // Set up thread-specific value's memory pointer before invoking constructor, in case any function it calls
+ // needs to access the value, to avoid recursion.
+ owner->setInTLS(this);
+ new (NotNull, storagePointer()) T;
+ }
+
+ ~Data()
+ {
+ storagePointer()->~T();
+ owner->setInTLS(nullptr);
+ }
+
+ PointerType storagePointer() const { return const_cast<PointerType>(reinterpret_cast<const T*>(&m_storage)); }
+
+ typename std::aligned_storage<sizeof(T), std::alignment_of<T>::value>::type m_storage;
ThreadSpecific<T, canBeGCThread>* owner;
};
+ T* get();
+ T* set();
+ void setInTLS(Data*);
+ void static THREAD_SPECIFIC_CALL destroy(void* ptr);
+
#if USE(PTHREADS)
pthread_key_t m_key;
#elif OS(WINDOWS)
@@ -156,17 +171,14 @@
{
Data* data = ""
if (data)
- return data->value;
- RELEASE_ASSERT(canBeGCThread == CanBeGCThread::True || !mayBeGCThread());
+ return data->storagePointer();
return nullptr;
}
template<typename T, CanBeGCThread canBeGCThread>
-inline void ThreadSpecific<T, canBeGCThread>::set(T* ptr)
+inline void ThreadSpecific<T, canBeGCThread>::setInTLS(Data* data)
{
- RELEASE_ASSERT(canBeGCThread == CanBeGCThread::True || !mayBeGCThread());
- ASSERT(!get());
- pthread_setspecific(m_key, new Data(ptr, this));
+ pthread_setspecific(m_key, data);
}
#elif OS(WINDOWS)
@@ -232,17 +244,13 @@
{
Data* data = ""
if (data)
- return data->value;
- RELEASE_ASSERT(canBeGCThread == CanBeGCThread::True || !mayBeGCThread());
+ return data->storagePointer();
return nullptr;
}
template<typename T, CanBeGCThread canBeGCThread>
-inline void ThreadSpecific<T, canBeGCThread>::set(T* ptr)
+inline void ThreadSpecific<T, canBeGCThread>::setInTLS(Data* data)
{
- RELEASE_ASSERT(canBeGCThread == CanBeGCThread::True || !mayBeGCThread());
- ASSERT(!get());
- Data* data = "" Data(ptr, this);
FlsSetValue(flsKeys()[m_index], data);
}
@@ -261,21 +269,20 @@
pthread_setspecific(data->owner->m_key, ptr);
#endif
- data->value->~T();
- fastFree(data->value);
-
-#if USE(PTHREADS)
- pthread_setspecific(data->owner->m_key, 0);
-#elif OS(WINDOWS)
- FlsSetValue(flsKeys()[data->owner->m_index], 0);
-#else
-#error ThreadSpecific is not implemented for this platform.
-#endif
-
delete data;
}
template<typename T, CanBeGCThread canBeGCThread>
+inline T* ThreadSpecific<T, canBeGCThread>::set()
+{
+ RELEASE_ASSERT(canBeGCThread == CanBeGCThread::True || !mayBeGCThread());
+ ASSERT(!get());
+ Data* data = "" Data(this); // Data will set itself into TLS.
+ ASSERT(get() == data->storagePointer());
+ return data->storagePointer();
+}
+
+template<typename T, CanBeGCThread canBeGCThread>
inline bool ThreadSpecific<T, canBeGCThread>::isSet()
{
return !!get();
@@ -284,15 +291,9 @@
template<typename T, CanBeGCThread canBeGCThread>
inline ThreadSpecific<T, canBeGCThread>::operator T*()
{
- T* ptr = static_cast<T*>(get());
- if (!ptr) {
- // Set up thread-specific value's memory pointer before invoking constructor, in case any function it calls
- // needs to access the value, to avoid recursion.
- ptr = static_cast<T*>(fastZeroedMalloc(sizeof(T)));
- set(ptr);
- new (NotNull, ptr) T;
- }
- return ptr;
+ if (T* ptr = get())
+ return ptr;
+ return set();
}
template<typename T, CanBeGCThread canBeGCThread>
@@ -307,19 +308,8 @@
return *operator T*();
}
-#if USE(WEB_THREAD)
-template<typename T, CanBeGCThread canBeGCThread>
-inline void ThreadSpecific<T, canBeGCThread>::replace(T* newPtr)
-{
- ASSERT(newPtr);
- Data* data = ""
- ASSERT(data);
- data->value->~T();
- fastFree(data->value);
- data->value = newPtr;
-}
-#endif
-
} // namespace WTF
+using WTF::ThreadSpecific;
+
#endif // WTF_ThreadSpecific_h
Modified: trunk/Source/WebCore/ChangeLog (220547 => 220548)
--- trunk/Source/WebCore/ChangeLog 2017-08-10 21:06:04 UTC (rev 220547)
+++ trunk/Source/WebCore/ChangeLog 2017-08-10 21:19:51 UTC (rev 220548)
@@ -1,3 +1,31 @@
+2017-08-09 Yusuke Suzuki <utatane....@gmail.com>
+
+ [WTF] ThreadSpecific should not introduce additional indirection
+ https://bugs.webkit.org/show_bug.cgi?id=175187
+
+ Reviewed by Mark Lam.
+
+ We drop ThreadSpecific::replace feature which is only used by
+ Web thread. Instead, we use ThreadSpecific<std::unique_ptr<T>> here.
+
+ While this std::unique_ptr<T> shares one instance between main thread
+ and web thread, this is the same to the current implementation. It is
+ safe because the web thread never finishes.
+
+ And for non-web thread implementation, we just use ThreadSpecific<T>,
+ since it is the most efficient.
+
+ * platform/ThreadGlobalData.cpp:
+ (WebCore::ThreadGlobalData::ThreadGlobalData):
+ (WebCore::ThreadGlobalData::setWebCoreThreadData):
+ (WebCore::threadGlobalData):
+ We also drop StringImpl::empty() call since it is not necessary now:
+ StringImpl::empty data is statically initialized by using constexpr.
+
+ * platform/ThreadGlobalData.h:
+ We make it FAST_ALLOCATED since it is previously allocated by fast malloc
+ in ThreadSpecific.
+
2017-08-10 Michael Catanzaro <mcatanz...@igalia.com>
REGRESSION(r220515) [GTK][CMake] Build with ENABLE_GEOLOCATION fails on Debian Jessie
Modified: trunk/Source/WebCore/platform/ThreadGlobalData.cpp (220547 => 220548)
--- trunk/Source/WebCore/platform/ThreadGlobalData.cpp 2017-08-10 21:06:04 UTC (rev 220547)
+++ trunk/Source/WebCore/platform/ThreadGlobalData.cpp 2017-08-10 21:19:51 UTC (rev 220548)
@@ -43,11 +43,6 @@
namespace WebCore {
-ThreadSpecific<ThreadGlobalData>* ThreadGlobalData::staticData;
-#if USE(WEB_THREAD)
-ThreadGlobalData* ThreadGlobalData::sharedMainThreadStaticData;
-#endif
-
ThreadGlobalData::ThreadGlobalData()
: m_cachedResourceRequestInitiators(std::make_unique<CachedResourceRequestInitiators>())
, m_eventNames(EventNames::create())
@@ -66,7 +61,6 @@
// point to call methods that internally perform a one-time initialization that is not
// threadsafe.
Thread::current();
- StringImpl::empty();
}
ThreadGlobalData::~ThreadGlobalData()
@@ -87,34 +81,55 @@
}
#if USE(WEB_THREAD)
+static ThreadSpecific<std::unique_ptr<ThreadGlobalData>>* staticData { nullptr };
+static ThreadGlobalData* sharedMainThreadStaticData { nullptr };
+
void ThreadGlobalData::setWebCoreThreadData()
{
ASSERT(isWebThread());
- ASSERT(&threadGlobalData() != ThreadGlobalData::sharedMainThreadStaticData);
+ ASSERT(&threadGlobalData() != sharedMainThreadStaticData);
// Set WebThread's ThreadGlobalData object to be the same as the main UI thread.
- ThreadGlobalData::staticData->replace(ThreadGlobalData::sharedMainThreadStaticData);
+ // The web thread never finishes, and we expect the main thread to also never finish.
+ // Hence, it is safe to store the same ThreadGlobalData pointer in a thread specific std::unique_ptr.
+ // FIXME: Make ThreadGlobalData RefCounted for web thread.
+ // https://bugs.webkit.org/show_bug.cgi?id=175439
+ (**staticData).reset(sharedMainThreadStaticData);
- ASSERT(&threadGlobalData() == ThreadGlobalData::sharedMainThreadStaticData);
+ ASSERT(&threadGlobalData() == sharedMainThreadStaticData);
}
-#endif
-ThreadGlobalData& threadGlobalData()
+ThreadGlobalData& threadGlobalData()
{
-#if USE(WEB_THREAD)
- if (UNLIKELY(!ThreadGlobalData::staticData)) {
- ThreadGlobalData::staticData = new ThreadSpecific<ThreadGlobalData>;
+ if (UNLIKELY(!staticData)) {
+ staticData = new ThreadSpecific<std::unique_ptr<ThreadGlobalData>>;
+ auto& result = **staticData;
+ ASSERT(!result);
+ result.reset(new ThreadGlobalData());
// WebThread and main UI thread need to share the same object. Save it in a static
// here, the WebThread will pick it up in setWebCoreThreadData().
if (pthread_main_np())
- ThreadGlobalData::sharedMainThreadStaticData = *ThreadGlobalData::staticData;
+ sharedMainThreadStaticData = result.get();
+ return *result;
}
- return **ThreadGlobalData::staticData;
+
+ auto& result = **staticData;
+ if (!result)
+ result.reset(new ThreadGlobalData());
+ return *result;
+}
+
#else
- if (!ThreadGlobalData::staticData)
- ThreadGlobalData::staticData = new ThreadSpecific<ThreadGlobalData>;
- return **ThreadGlobalData::staticData;
-#endif
+
+static ThreadSpecific<ThreadGlobalData>* staticData { nullptr };
+
+ThreadGlobalData& threadGlobalData()
+{
+ if (UNLIKELY(!staticData))
+ staticData = new ThreadSpecific<ThreadGlobalData>;
+ return **staticData;
}
+#endif
+
} // namespace WebCore
Modified: trunk/Source/WebCore/platform/ThreadGlobalData.h (220547 => 220548)
--- trunk/Source/WebCore/platform/ThreadGlobalData.h 2017-08-10 21:06:04 UTC (rev 220547)
+++ trunk/Source/WebCore/platform/ThreadGlobalData.h 2017-08-10 21:19:51 UTC (rev 220548)
@@ -29,9 +29,6 @@
#include <wtf/text/StringHash.h>
-#include <wtf/ThreadSpecific.h>
-using WTF::ThreadSpecific;
-
namespace WebCore {
class QualifiedNameCache;
@@ -44,6 +41,7 @@
class ThreadGlobalData {
WTF_MAKE_NONCOPYABLE(ThreadGlobalData);
+ WTF_MAKE_FAST_ALLOCATED;
public:
WEBCORE_EXPORT ThreadGlobalData();
WEBCORE_EXPORT ~ThreadGlobalData();
@@ -80,10 +78,6 @@
std::unique_ptr<TECConverterWrapper> m_cachedConverterTEC;
#endif
- WEBCORE_EXPORT static ThreadSpecific<ThreadGlobalData>* staticData;
-#if USE(WEB_THREAD)
- WEBCORE_EXPORT static ThreadGlobalData* sharedMainThreadStaticData;
-#endif
WEBCORE_EXPORT friend ThreadGlobalData& threadGlobalData();
};
