Bug 25348 – Change WTF::ThreadIdentifier to be an actual (but wrapped) thread id, remove ThreadMap.

• Component: Web Template Framework
• Status: RESOLVED WONTFIX

[Dmitry Titov]

WTF defines ThreadIdentifier as a platform-independent thread ID. It does it by keeping platform-dependent ThreadMap which maps native thread IDs into sequential integers. The mapping happens 'opportunistically', either when thread is created via WTF or when currentThread() is called first time on a thread that was not created via WTF. The mapping is removed when (and if) WTF::detachThread(threadID) is called.

These mappings may leak and get out of sync because detachThread() is not enforced in any way, on some platforms has empty implementation, and if some code (like destructors of thread-specific data) runs after detachThread is invoked, it creates a new mapping which is never removed. See bug 25138 for particular scenario.

Solution:
Remove ThreadMap and all the mappings. Make ThreadIdentifier a wrapper around platform-specific thread ID which implements comparison operators needed in the code. Fortunately, all platform thread IDs are either integers or opaque pointers so the ThreadIdentifier is easily copyable. This simplifies the code because there is not need to maintain mappings, and makes ThreadIdentifier valid even while running pthread's thread-specific destructors.

[Dmitry Titov]

*** Bug 25138 has been marked as a duplicate of this bug. ***

[Dmitry Titov]

Created attachment 29715 [details]
Proposed patch

[Alexey Proskuryakov]

Comment on attachment 29715 [details]
Proposed patch

+// so you can do threadId = 0 to 'invalidate' the ThreadIdentifier.

Why not make a function for this?

+// Returns 0 if thread creation failed.
+// The thread name must be a literal since on some platforms it's passed in to the thread.
+ThreadIdentifier createThread(ThreadFunction, void*, const char* threadName);

Looks like the comment should say "returns an invalid identifier..."

+bool ThreadIdentifier::operator==(const ThreadIdentifier&) const { return false; }
+bool ThreadIdentifier::operator!=(const ThreadIdentifier&) const { return false; }

If there is only one thread, they are all equal - operator== should return true in ThreadingNone.

+        (WebCore::libxmlLoaderThread): use DEFINE_STATIC_LOCAL and accessor function for static thread id.

It's better to tell why, not what changed (even though is rather obvious in this case, I did have to pause and think).

+static ThreadIdentifier& libxmlLoaderThread() {

The brace should go to the next line.

A major consequence of this change is that ThreadIdentifiers are no longer invalidated automatically - previously, pthreadHandleForIdentifier() would return 0 for a ThreadIdentifier that was created for a destroyed thread. Now there is a danger of using it after a thread is destroyed, if another thread gets the same platform ID in the future. I think that it's ok - but how did you check that no code relies on that?

[Dmitry Titov]

(In reply to comment #3)
> (From update of attachment 29715 [details] [review])
> +// so you can do threadId = 0 to 'invalidate' the ThreadIdentifier.
> 
> Why not make a function for this?

:-) I was hesitating on this one because it's more changes (threadId is often initialized by 0). In fact, you are right and it's logical to have invalidate()/isValid() pair, or assignment/check for 0 pair, but not a mix of these two. Removed possibility to assign 0 and added invalidate().

> 
> +// Returns 0 if thread creation failed.
> +// The thread name must be a literal since on some platforms it's passed in to
> the thread.
> +ThreadIdentifier createThread(ThreadFunction, void*, const char* threadName);
> 
> Looks like the comment should say "returns an invalid identifier..."
> 

Done.

> +bool ThreadIdentifier::operator==(const ThreadIdentifier&) const { return
> false; }
> +bool ThreadIdentifier::operator!=(const ThreadIdentifier&) const { return
> false; }
> 
> If there is only one thread, they are all equal - operator== should return true
> in ThreadingNone.
> 

Done.

> +        (WebCore::libxmlLoaderThread): use DEFINE_STATIC_LOCAL and accessor
> function for static thread id.
> 
> It's better to tell why, not what changed (even though is rather obvious in
> this case, I did have to pause and think).

Done. 

> 
> +static ThreadIdentifier& libxmlLoaderThread() {
> 
> The brace should go to the next line.
> 

Done.

> A major consequence of this change is that ThreadIdentifiers are no longer
> invalidated automatically - previously, pthreadHandleForIdentifier() would
> return 0 for a ThreadIdentifier that was created for a destroyed thread. Now
> there is a danger of using it after a thread is destroyed, if another thread
> gets the same platform ID in the future. I think that it's ok - but how did you
> check that no code relies on that?
> 

That's a good point. I thought how could I verify that code does not de-facto rely on that. It shouldn't, because OSes typically don't makes guarantees about reusing handles of any sort, but it could grew to depend on previous implementation. I've made comparison operators private, recompiled, and then looked at all the code locations that broke (ActiveDOMObject, SQLiteDatabase, DatabaseTracker, XMLTokenizerLibxml2). They all seem to use a pattern of creating an object on a certain thread, with lifespan enclosed by the thread - and then check incoming APIs that they are happening at the same thread. This particular usage is fine with reusing thread ids. So I am pretty certain that there is no explicit assumption that ids are unique in WebKit itself. Of course, there could be a bug hiding somewhere (including embedders that use the same WTF) that 'rely' on uniqueness by accident. But in this case, it could be very hard to ASSERT in some programmatic way - since the difference in behavior may be quite subtle and effect of it could be unexpected. So I'd give it a shot. Please let me know if you have some suggestion here!

[Dmitry Titov]

Created attachment 29832 [details]
Modified after code review

Modified according to Alexey's comments ^^^

[Alexey Proskuryakov]

Comment on attachment 29832 [details]
Modified after code review

+    static ThreadIdentifier create(PlatformThreadIdentifier platformId)
+    {
+        return ThreadIdentifier(platformId);
+    }

This was not in the original patch - and it looks confusing to me. When I saw "return ThreadIdentifier::create(threadHandle)", I thought the class was now refcounted.

Would marking the constructor as explicit accomplish what you wanted in a simpler way perhaps?

[Dmitry Titov]

Created attachment 29836 [details]
create(..) -> explicit ctor

That's right, explicit ctor is way better. Updated the patch.

[Alexey Proskuryakov]

Comment on attachment 29836 [details]
create(..) -> explicit ctor

+    ThreadIdentifier(const ThreadIdentifier& another)
+        : m_platformId(another.m_platformId)
+    {
+    }

Sorry, I didn't notice it before - if you are adding a non-default copy constructor, you should also add a matching assignment operator. But since this does the same as default one, I think that it can be simply removed.

You may need to also update MSVC JavaScriptCore exports, now that it's a separate dll.

r=me

[Dmitry Titov]

While building on Windows in order to update .def files, I found a bug in ThreadingWin.cpp (I missed the fact that in Win implementation, we didn't use sequential counter, rather the ThreadMap was Win32_thread_id -> threadHandle. 
Also an interesting issue - public Safari 4.0 beta apparently uses WTF threading functions, so changing their decorated symbol names (because of ThreadIdentifier parameter/return value is now decorated differently) makes run-safari fail since Safari can't load WTF threading functions from Webkit.dll. OSX Safari is fine though.

I'm attaching the patch with removed copy constructor and fixes in ThreadingWin.cpp. My plan so far is to make sure layout test run fine on Windows and then check it in. However, I worry about Win Safari not being able to run with nightly builds in case it won't be able to update. 

[Dmitry Titov]

Created attachment 29877 [details]
Almost ready for landing - needs .def files.

[Alexey Proskuryakov]

We shouldn't break Windows nightly builds, that's for sure. CC'ing Adam, who may have an idea how to handle this.

[Adam Roben (:aroben)]

Comment on attachment 29877 [details]
Almost ready for landing - needs .def files.

>  int waitForThreadCompletion(ThreadIdentifier threadID, void** result)
>  {
> -    ASSERT(threadID);
> +    ASSERT(threadID.isValid());
>      
> -    HANDLE threadHandle = threadHandleForIdentifier(threadID);
> +    unsigned threadIdentifier = threadID.platformId();
> +    HANDLE threadHandle = OpenThread(SYNCHRONIZE, FALSE, threadIdentifier);
>      if (!threadHandle)
>          LOG_ERROR("ThreadIdentifier %u did not correspond to an active thread when trying to quit", threadID);

Seems like this isn't an error condition anymore. It just means that the thread exited before waitForThreadCompletion was called.

I think your ChangeLog should mention these changes to ThreadingWin (closing the handle and the re-opening it later) explicitly. Maybe your changes to the other ports could use more explicit explanations, too (though I haven't read those changes as carefully).

Can we provide (deprecated) versions of the Threading.h functions that match the old signatures for backwards-compatibility with Safari 4 Public Beta? Seems like we'd just need to have versions that continue to take a PlatformThreadIdentifier.

[Dmitry Titov]

Created attachment 30078 [details]
Latest - with Win .def files and deprecated functions.

Thanks a lot for detailed review and advise!
I think I've got it right now, but removing Alexey's r+ and requesting another look since there are additional changes (hope it's the last!):
- added 'deprecated' functions for Windows, which take the PlatformThreadIdetifier. Had to change their names too because otherwise some of them only differ in returning type.
- updated .def files for Windows. Depricated functions are now exported with rename from WebKit.def so they match their old names. Verified that run-safari works fine now.

I think this is ready for another look. Thanks!

[Alexey Proskuryakov]

Comment on attachment 30078 [details]
Latest - with Win .def files and deprecated functions.

+        which it uses. Next time Safari 4 builds, it will pick up new functions and the depricated ones

"deprecated"

+// In WebKit.def file, these functions are 'renamed' so their name does not have 'Depricated' part - this

Ditto.

\ No newline at end of file

Please add one (to three files).

+// Platform-independent wrapper of ThreadId. Assignable and copyable.

There is no such thing as ThreadId.

+// It happens that for all current implementations '0' works fine as 'invalid value'.
+// Comparing is delegated to the platform implementations because it needs to
+// compare in a platform-dependent way.

I'm not sure if these comments add anything - e.g., it's immediately obvious that comparing is implemented differently for different platforms by looking at the code alone. Though it is true that it may be slightly surprising that the only substantial difference from a typedef is a custom operator==.

But I'm still somewhat worried about losing the guarantee that a ThreadIdentifier's native thread id is null after the thread exits. It is probably worth adding a comment that ThreadIdentifier is only valid for as long as its thread is alive, and after that, platformId may be invalid, or may reference a completely unrelated thread.

r=me

[Dmitry Titov]

(In reply to comment #14)
> "deprecated"

Fixed.

> have 'Depricated' part - this
> Ditto.

Fixed.

> \ No newline at end of file
> Please add one (to three files).

Fixed.

> +// Platform-independent wrapper of ThreadId. Assignable and copyable.
> 
> There is no such thing as ThreadId.

Replaced with generic 'thread id'.

> +// It happens that for all current implementations '0' works fine as 'invalid
> value'.
> +// Comparing is delegated to the platform implementations because it needs to
> +// compare in a platform-dependent way.
> 
> I'm not sure if these comments add anything - e.g., it's immediately obvious
> that comparing is implemented differently for different platforms by looking at
> the code alone. Though it is true that it may be slightly surprising that the
> only substantial difference from a typedef is a custom operator==.
> 
> But I'm still somewhat worried about losing the guarantee that a
> ThreadIdentifier's native thread id is null after the thread exits. It is
> probably worth adding a comment that ThreadIdentifier is only valid for as long
> as its thread is alive, and after that, platformId may be invalid, or may
> reference a completely unrelated thread.

Removed comment about implementation, added comment about validity scope and possible reference to another thread.

Fixed ChangeLogs, whitespaces in empty lines and committed: http://trac.webkit.org/changeset/43366

[Mark Rowe (bdash)]

This caused bug 25640, so I will probably be rolling this change out.  Conveniently, no-one bothered to close this bug when the patch was landed so I don't have to reopen it to do so.

[Mark Rowe (bdash)]

I rolled it out in r43392.

[Dmitry Titov]

Created attachment 30147 [details]
Another attempt. Fixed Safari 4 beta on OSX issue.

Same patch, with fix for Safari 4 beta on OSX issue. 
The fix is in ThreadingPthreads.cpp, at the end - I've added a (deprecated) function waitForThreadCompletion that takes uint32_t instead of ThreadIdentifier. It has the same decorated name as before so Safari 4 beta finds the function when it loads it from JavaScriptCore. The other functions, like CurrentThread() happen to have the same decorated names so no deprecated functions for them is needed. 
The new ThreadIdentifier is stored inside Safari 4 as uint32_t, and then we cast it back to pointer. 
Updated JavaScriptCore.exp and JavaScriptCore/ChangeLog accordingly, tested with Safari 4 beta.

[Dmitry Titov]

I flipped r? again, however the bulk of the patch is the same, there is an added function with old prototype as described above.

[Alexey Proskuryakov]

Comment on attachment 30147 [details]
Another attempt. Fixed Safari 4 beta on OSX issue.

+// The only way to obtain a valid ThreadIdentifier is from createThread(...) or currentThread() functions.

I'm not a fan of this new comment - it will likely stay there even if we add other ways.

Looks fine, r=me

[Dmitry Titov]

Fixed comment and landed: http://trac.webkit.org/changeset/43507

[Dmitry Titov]

The fix was reverted inhttp://trac.webkit.org/changeset/43663

The reason is the crash in nightlies on ppc with Safari 4. Safari 4 uses WTF and change in WTF threading API breaks binary compatibility with the shipped Safari 4 executable. 

No simple workaround was found since Safari loads the same JavaScriptCore.framework as WebCore and some functions only differ in return type which is not a part of decorated name so it's impossible to surface 2 sets of functions with the same names as was done for Win and x86 OSX.

[David Levin]

Comment on attachment 30078 [details]
Latest - with Win .def files and deprecated functions.

Removed r+ to get this out of the commit queue.

[David Levin]

Comment on attachment 29836 [details]
create(..) -> explicit ctor

Removed r+ to get this out of the commit queue.

[Alexey Proskuryakov]

(In reply to comment #14)
> But I'm still somewhat worried about losing the guarantee that a
> ThreadIdentifier's native thread id is null after the thread exits.

See also: <http://www.nabble.com/New-FAQ-entry-re-why-pthread_t-is-a-struct-td14358257.html>

"When pthread_t is a simple pointer to a struct some very difficult to debug problems arise from the process of freeing and later allocing thread structs because new pthread_t handles can acquire the identity of previously detached threads. The change to a struct was made, along with some changes to their internal managment, in order to guarantee (for practical applications) that the pthread_t handle will be unique over the  life of the running process."

Maybe it is really not good to lose this guarantee?

[Alexey Proskuryakov]

At the same time, the same FAQ goes on to mention that many pthreads implementations guarantee pthread_t pointer uniqueness - it would be interesting to know if that's the case on platforms we care about.

[Dmitry Titov]

(In reply to comment #26)
> At the same time, the same FAQ goes on to mention that many pthreads
> implementations guarantee pthread_t pointer uniqueness - it would be
> interesting to know if that's the case on platforms we care about.
> 

I verified that this program:

#include <iostream>

static void* thread_proc(void* arg)
{  return 0; }

int main (int argc, char * const argv[]) {
    pthread_t threadHandle;
    for (int i = 0; i < 10; ++i) {
     pthread_create(&threadHandle, 0, thread_proc, 0);
     std::cout << threadHandle << "\n";
     pthread_join(threadHandle, 0);
    }
    return 0;
}

prints the same number 10 times on x86 Leopard :-)

I agree with the worry that stale thread ids could start matching wrong threads and occasionally cause a hard-to-find threading issue. Hm. Initially, the motivation for this change was the implementation of workers in Chrome+v8 where the mapping schema didn't work because of thread-specific destructors. I'll see if it can be fixed other way.

[Alexey Proskuryakov]

For me, the best thing about this change was getting rid of a global map protected with a mutex.

> prints the same number 10 times on x86 Leopard :-)

Thanks for checking! That's definitely a platform we care about.

[Dmitry Titov]

Resolving WONTFIX. While the fix can eliminate a mutex-protected table of ids and some edge issues with maintaining that table, it also makes it possible to recycle thread ids from terminated thread to a new one, which may cause some hard-to-debug issues since code in general relies on comparing ids to make sure the thread is the same one.

[Mark Rowe (bdash)]

*** Bug 30922 has been marked as a duplicate of this bug. ***

[Peter Kasting]

Given the incoming duped bug, is it still appropriate to have this be WONTFIX?  Perhaps one of the solutions on that bug would be appropriate?

[Jens Alfke]

I think something needs to be done about this. As I said in my duped bug, Chromium is spending about 10% of its garbage-collection time in WTF::currentThread, as a result of the cleanup code for released DOM bindings.

Is pthread_getspecific fast enough to use as a replacement?

[Dmitry Titov]

The code that you mentioned (v8 GC ensureDeref) was written a while ago to deref DOM objects created in Worker threads - this was before we realized it's too hard to make v8 running in separate threads and switched to a worker-in-separate-process solution.

I don't know a scenario today when wrappers for dom objects are created on different threads. It is possible that the code querying currentThread() may be simply deleted (together with perf problem).

[Alexey Proskuryakov]

> Is pthread_getspecific fast enough to use as a replacement?

I'm not sure how it could be used - we need to manipulate thread identifiers of other threads, but thread-specific values are unreachable from other threads, as far as I know.