Put ThreadGlobalData in Thread #521

Constellation · 2022-05-05T10:58:04Z

`04f2a2b`

Put ThreadGlobalData in Thread
https://bugs.webkit.org/show_bug.cgi?id=240116

Reviewed by Darin Adler and Mark Lam.

This patch puts ThreadGlobalData into Thread's m_clientData field.
Thread is stored in fast TLS, so accessing to this field is quite fast
compared to the current ThreadSpecific one.

At the same time, this change can remove a hack in ThreadGlobalData.
Previously worker thread needs to tear down ThreadGlobalData explicitly
because Thread::current() can be destroyed earlier than ThreadGlobalData.
In that case, EventNames etc., which accesses to AtomString's destruction
can have problem because of lack of Thread::current() access. But since
we now move it to Thread, we can control how it is destroyed with Thread::current()
precisely, and we can explicitly destroy it before Thread::current() is fully
cleared. So we do not need to call it explicitly anymore.
Currently, we are calling it just to make ThreadGlobalData destroyed for debugging.

* Source/WebCore/PAL/pal/ThreadGlobalData.cpp:
(PAL::ThreadGlobalData::ThreadGlobalData):
(PAL::ThreadGlobalData::destroy): Deleted.
* Source/WebCore/PAL/pal/ThreadGlobalData.h:
(PAL::ThreadGlobalData::ThreadGlobalData::cachedConverterICU): Deleted.
* Source/WTF/wtf/Threading.h:
(WTF::Thread::Thread):
* Source/WTF/wtf/posix/ThreadingPOSIX.cpp:
(WTF::Thread::destructTLS):
* Source/WTF/wtf/win/ThreadingWin.cpp:
(WTF::Thread::ThreadHolder::~ThreadHolder):
* Source/WebCore/platform/ThreadGlobalData.cpp:
(WebCore::ThreadGlobalData::destroy):
(WebCore::ThreadGlobalData::setWebCoreThreadData):
(WebCore::threadGlobalData):

Canonical link: https://commits.webkit.org/250571@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294213 268f45cc-cd09-0410-ab3c-d52691b4dbfc

cdumez · 2022-05-05T22:39:16Z

EWS failures look real:

stderr:
ASSERTION FAILED: The string being removed is an atom in the string table of an other thread!
iterator != atomStringTable.end()
/Volumes/Data/worker/macOS-AppleSilicon-Big-Sur-Debug-Build-EWS/build/Source/WTF/wtf/text/AtomStringImpl.cpp(451) : static void WTF::AtomStringImpl::remove(WTF::AtomStringImpl *)
1   0x14231dbf8 WTFCrash
2   0x1423261dc WTF::AtomStringImpl::remove(WTF::AtomStringImpl*)
3   0x1423faf8c WTF::StringImpl::~StringImpl()
4   0x1423fb330 WTF::StringImpl::~StringImpl()
5   0x1423fb35c WTF::StringImpl::destroy(WTF::StringImpl*)
6   0x11bf28a34 WTF::StringImpl::deref()
7   0x11c398018 WTF::DefaultRefDerefTraits<WTF::StringImpl>::derefIfNotNull(WTF::StringImpl*)
8   0x11bf28a7c WTF::RefPtr<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl>, WTF::DefaultRefDerefTraits<WTF::StringImpl> >::~RefPtr()
9   0x11c0115a0 WTF::RefPtr<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl>, WTF::DefaultRefDerefTraits<WTF::StringImpl> >::~RefPtr()
10  0x11bf28c00 WTF::String::~String()
11  0x11bfc7c90 WTF::String::~String()
12  0x11be30d20 WTF::AtomString::~AtomString()
13  0x11bebf470 WTF::AtomString::~AtomString()

Constellation · 2022-05-06T00:07:19Z

Yup, this looks like a real issue.

Constellation · 2022-05-06T04:45:15Z

Interesting, we are still having some crashes, looking.

darinadler

Might be even better to not have to dereference a second pointer at all, and store all this inside the Thread object, allocated with an appropriate size.

I’m not sure the reference counting is needed here. This is a nice improvement.

darinadler · 2022-05-08T21:09:36Z

Source/WTF/wtf/Threading.h

@@ -109,6 +109,11 @@ class WTF_CAPABILITY("is current") Thread : public ThreadSafeRefCounted<Thread>
    friend class ThreadGroup;
    friend WTF_EXPORT_PRIVATE void initialize();

+    class ApplicationData : public ThreadSafeRefCounted<ApplicationData> {


I don’t think application is a great term to use here. I understand we need some abstract way to refer to WebCore from JavaScriptCore, but application has a pretty specific meaning as a term of art on platforms like iOS, and WebCore is not one of those things.

Why does this need to be reference counted? What is the second owner besides the Thread?

How about ClientData?

ClientData sounds good.

Why does this need to be reference counted? What is the second owner besides the Thread?

Because WebThread is sharing it with the main thread. So, in iOS, WebKitLegacy, it is shared by multiple threads (WebThread and OS main thread).

darinadler · 2022-05-08T21:10:09Z

Source/WebCore/platform/ThreadGlobalData.cpp

+    if (LIKELY(applicationData))
+        return *static_cast<ThreadGlobalData*>(applicationData);
+
+    auto data = adoptRef(*new ThreadGlobalData());


No need to add the () here.

darinadler · 2022-05-08T21:11:40Z

Source/WebCore/platform/ThreadGlobalData.cpp

+    if (LIKELY(applicationData))
+        return *static_cast<ThreadGlobalData*>(applicationData);
+
+    auto data = adoptRef(*new ThreadGlobalData());


No need to add the () here.

Nice, dropped.

MenloDorian · 2022-05-08T21:22:20Z

Source/WTF/wtf/Threading.h

@@ -109,6 +109,11 @@ class WTF_CAPABILITY("is current") Thread : public ThreadSafeRefCounted<Thread>
    friend class ThreadGroup;
    friend WTF_EXPORT_PRIVATE void initialize();

+    class ApplicationData : public ThreadSafeRefCounted<ApplicationData> {


How about ClientData?

MenloDorian · 2022-05-08T21:41:40Z

Source/WebCore/platform/ThreadGlobalData.cpp

@@ -76,42 +63,42 @@ void ThreadGlobalData::setWebCoreThreadData()
    ASSERT(&threadGlobalData() != sharedMainThreadStaticData);

    // Set WebThread's ThreadGlobalData object to be the same as the main UI thread.
-    **staticData = adoptRef(sharedMainThreadStaticData);
+    Thread::current().m_applicationData = adoptRef(sharedMainThreadStaticData);


ASSERT(&threadGlobalData() != sharedMainThreadStaticData); implies that Thread::current().m_applicationData should already be pointing to sharedMainThreadStaticData. Do we still need this ThreadGlobalData::setWebCoreThreadData function?

Maybe remove this assignment, and rename this function to assertWebCoreThreadData() instead?

Ah, no. ASSERT(&threadGlobalData() != sharedMainThreadStaticData); means that Thread::current().m_applicationData is not pointing to sharedMainThreadStaticData.

Constellation

Thanks!

Constellation · 2022-05-09T06:19:27Z

Source/WebCore/platform/ThreadGlobalData.cpp

+    if (LIKELY(applicationData))
+        return *static_cast<ThreadGlobalData*>(applicationData);
+
+    auto data = adoptRef(*new ThreadGlobalData());


Nice, dropped.

Constellation · 2022-05-09T06:31:45Z

Source/WebCore/platform/ThreadGlobalData.cpp

@@ -76,42 +63,42 @@ void ThreadGlobalData::setWebCoreThreadData()
    ASSERT(&threadGlobalData() != sharedMainThreadStaticData);

    // Set WebThread's ThreadGlobalData object to be the same as the main UI thread.
-    **staticData = adoptRef(sharedMainThreadStaticData);
+    Thread::current().m_applicationData = adoptRef(sharedMainThreadStaticData);


Ah, no. ASSERT(&threadGlobalData() != sharedMainThreadStaticData); means that Thread::current().m_applicationData is not pointing to sharedMainThreadStaticData.

Constellation · 2022-05-09T06:31:57Z

Source/WebCore/platform/ThreadGlobalData.cpp

+    if (LIKELY(applicationData))
+        return *static_cast<ThreadGlobalData*>(applicationData);
+
+    auto data = adoptRef(*new ThreadGlobalData());


Constellation

TESTING (Oops, I pushed while testing some GitHub UI).

webkit-early-warning-system · 2022-05-14T20:50:00Z

Commit message contains (OOPS!), blocking PR #521

webkit-early-warning-system · 2022-05-15T00:48:17Z

Committed r294213 (250571@main): https://commits.webkit.org/250571@main

Reviewed commits have been landed. Closing PR #521 and removing active labels.

Constellation self-assigned this May 5, 2022

Constellation added New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 5, 2022

Constellation force-pushed the eng/tls branch from e400f5d to 88104d6 Compare May 5, 2022 11:00

Constellation requested a review from cdumez May 5, 2022 11:05

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 5, 2022

Constellation marked this pull request as draft May 6, 2022 00:07

Constellation removed merging-blocked Applied to prevent a change from being merged New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 6, 2022

Constellation force-pushed the eng/tls branch from 88104d6 to ada9667 Compare May 6, 2022 00:28

Constellation added New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 6, 2022

Constellation marked this pull request as ready for review May 6, 2022 00:29

Constellation marked this pull request as draft May 6, 2022 04:45

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 6, 2022

Constellation removed merging-blocked Applied to prevent a change from being merged New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 7, 2022

Constellation force-pushed the eng/tls branch from ada9667 to 0e1bc16 Compare May 7, 2022 06:38

Constellation added New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 7, 2022

Constellation marked this pull request as ready for review May 7, 2022 22:16

Constellation requested a review from darinadler May 7, 2022 22:16

darinadler approved these changes May 8, 2022

View reviewed changes

MenloDorian approved these changes May 8, 2022

View reviewed changes

Constellation commented May 9, 2022

View reviewed changes

Constellation force-pushed the eng/tls branch from 0e1bc16 to a8a45e6 Compare May 9, 2022 06:36

Constellation commented May 11, 2022

View reviewed changes

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 11, 2022

Constellation added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels May 14, 2022

webkit-early-warning-system added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels May 14, 2022

Constellation removed merging-blocked Applied to prevent a change from being merged New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels May 14, 2022

Constellation force-pushed the eng/tls branch from a8a45e6 to 587be86 Compare May 14, 2022 23:30

Constellation added New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build merge-queue Applied to send a pull request to merge-queue labels May 14, 2022

webkit-early-warning-system closed this May 15, 2022

webkit-early-warning-system force-pushed the eng/tls branch from 587be86 to 04f2a2b Compare May 15, 2022 00:48

webkit-early-warning-system merged commit 04f2a2b into WebKit:main May 15, 2022

webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label May 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put ThreadGlobalData in Thread #521

Put ThreadGlobalData in Thread #521

Constellation commented May 5, 2022 •

edited by webkit-early-warning-system

cdumez commented May 5, 2022

Constellation commented May 6, 2022

Constellation commented May 6, 2022

darinadler left a comment

darinadler May 8, 2022

MenloDorian May 8, 2022

Constellation May 9, 2022

darinadler May 8, 2022

Constellation May 9, 2022

darinadler May 8, 2022

Constellation May 9, 2022

MenloDorian May 8, 2022

MenloDorian May 8, 2022

Constellation May 9, 2022

Constellation left a comment

Constellation May 9, 2022

Constellation May 9, 2022

Constellation May 9, 2022

Constellation left a comment •

edited

webkit-early-warning-system commented May 14, 2022

webkit-early-warning-system commented May 15, 2022

Put ThreadGlobalData in Thread #521

Put ThreadGlobalData in Thread #521

Conversation

Constellation commented May 5, 2022 • edited by webkit-early-warning-system

cdumez commented May 5, 2022

Constellation commented May 6, 2022

Constellation commented May 6, 2022

darinadler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Constellation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Constellation left a comment • edited

Choose a reason for hiding this comment

webkit-early-warning-system commented May 14, 2022

webkit-early-warning-system commented May 15, 2022

Constellation commented May 5, 2022 •

edited by webkit-early-warning-system

Constellation left a comment •

edited