Bug 22614 – Need to add Win32 implementation of ThreadSpecific.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jian Li]

Need to add Win32 implementation of ThreadSpecific.

[Jian Li]

Created attachment 25697 [details]
Proposed Patch

[Alexey Proskuryakov]

Comment on attachment 25697 [details]
Proposed Patch

> -#if USE(PTHREADS) || PLATFORM(WIN)
> +#if USE(PTHREADS) || PLATFORM(WIN) || PLATFORM(WIN_OS)

No need to keep PLATFORM(WIN), because it can only be set if PLATFORM(WIN_OS) is set.

> +#if USE(PTHREADS) || PLATFORM(WIN)
>      pthread_key_t m_key;

Similarly, I think this test should be just USE(PTHREADS).

> +template<typename T>
> +inline void ThreadSpecific<T>::destroy(void* ptr)

What calls destroy() when a thread exits? Per <http://msdn.microsoft.com/en-us/library/ms686801(VS.85).aspx>, data kept in per-thread storage must be destroyed manually in a DLL_THREAD_DETACH callback.

r-, because I think that per-thread data leaks.

[Jian Li]

Created attachment 25731 [details]
Proposed Patch

[Jian Li]

(In reply to comment #2)
> (From update of attachment 25697 [details] [review])
> > -#if USE(PTHREADS) || PLATFORM(WIN)
> > +#if USE(PTHREADS) || PLATFORM(WIN) || PLATFORM(WIN_OS)
> 
> No need to keep PLATFORM(WIN), because it can only be set if PLATFORM(WIN_OS)
> is set.
Removed.
> 
> > +#if USE(PTHREADS) || PLATFORM(WIN)
> >      pthread_key_t m_key;
> 
> Similarly, I think this test should be just USE(PTHREADS).
Removed.
> 
> > +template<typename T>
> > +inline void ThreadSpecific<T>::destroy(void* ptr)
> 
> What calls destroy() when a thread exits? Per
> <http://msdn.microsoft.com/en-us/library/ms686801(VS.85).aspx>, data kept in
> per-thread storage must be destroyed manually in a DLL_THREAD_DETACH callback.
> 
> r-, because I think that per-thread data leaks.
Though MSDN says that it is expected to call TlsFree in a DLL_THREAD_DETACH callback, it is not absolutely required. We can still call TlsFree in other place.
> 

[Alexey Proskuryakov]

Comment on attachment 25731 [details]
Proposed Patch

Sorry, I pasted a wrong MSDN link, it was supposed to be <http://msdn.microsoft.com/en-us/library/ms686997(VS.85).aspx>.

MSDN suggests calling TlsFree from DLL_PROCESS_DETACH, and it is indeed OK to call it manually, or even not to call it at all - but I'm not talking about leaking TLS keys. The problem is about leaking TLS data of each thread. In pthread implementation of this class, destroy() is called automatically for each ThreadSpecific object on thread exit, because the callback is registered by pthread_key_create() - but nothing calls destroy() in this patch.

[Jian Li]

Created attachment 26105 [details]
Proposed Patch

[Jian Li]

Thanks for pointing this out. I've fixed this problem. Please review my change again. Sorry for taking so long to reply since I am just back from a trip.

(In reply to comment #5)
> (From update of attachment 25731 [details] [review])
> Sorry, I pasted a wrong MSDN link, it was supposed to be
> <http://msdn.microsoft.com/en-us/library/ms686997(VS.85).aspx>.
> 
> MSDN suggests calling TlsFree from DLL_PROCESS_DETACH, and it is indeed OK to
> call it manually, or even not to call it at all - but I'm not talking about
> leaking TLS keys. The problem is about leaking TLS data of each thread. In
> pthread implementation of this class, destroy() is called automatically for
> each ThreadSpecific object on thread exit, because the callback is registered
> by pthread_key_create() - but nothing calls destroy() in this patch.
> 

[Alexey Proskuryakov]

Comment on attachment 26105 [details]
Proposed Patch

Are you suggesting the code for use on all Win32 ports, or only on those that don't use pthreads already? I can't tell for sure from the patch - I may be mistaken, but it seems that ifdefs are still confused a little.


> +        * JavaScriptCore.xcodeproj/project.pbxproj:

All code in ThreadSpecific.cpp is Windows-only, so it should be called ThreadSpecificWin.cpp, and it shouldn't be in this Mac-specific project file.

> + * Copyright (C) 2008 Apple Inc. All rights reserved.

I don't see any Apple code in this file, so you needn't assign copyright to Apple.

> +
> +#include "ThreadSpecific.h"

All cpp files must include config.h first.

> +#if PLATFORM(WIN_OS)

In ThreadSpecificWin.cpp, there will be no need for the ifdef.

> +void ThreadSpecificThreadExit() {

The brace should go on next line.

> +    ThreadSpecific<int>::ThreadExit();

I think that ThreadExit() function needn't be in the class - having to pass a dummy template parameter is ugly. I think you could make a separate class with the same layout, and static_cast to it - this will make the assumption of same layout explicit.

> +#if PLATFORM(WIN_OS)
> +// ThreadSpecificThreadExit should be called each time when a thread is detached.
> +extern void ThreadSpecificThreadExit();
> +#endif

Will calling this function be a JavaScriptCore API requirement? Then it should be in API/JSBase.h, and named differently, but that sounds like a particularly unfortunate requirement to impose on clients. Many libraries have initialization calls, but few need explicit shutdown, especially on each thread. I don't see a good solution, maybe we should discuss this on the mailing list.

> +    m_key = TlsAlloc();
> +    if (m_key == TLS_OUT_OF_INDEXES)
> +        CRASH();
> +
> +    int slot = InterlockedIncrement(&g_tls_key_count) - 1;

It seems that TLS is almost re-implemented here, in addition to using the platform-provided solution. I hope that there is a simpler solution.

The main issue here is that this basically changes JavaScriptCore API in an objectionable way - I don't have a proposal, but this definitely needs to be resolved.

Sorry for not reviewing this earlier, I overlooked the patch when it was added.

[Alexey Proskuryakov]

(In reply to comment #8)
> The main issue here is that this basically changes JavaScriptCore API in an
> objectionable way - I don't have a proposal, but this definitely needs to be
> resolved.

Oh, in fact, nothing in JavaScriptCore uses ThreadSpecific, so I do have a proposal. I think that threads created with WTF::createThread() should just clean up when exiting - as it is done in Win32+pthreads case in wtfThreadEntryPoint (ThreadingWin.cpp).

[Jian Li]

I've fixed most of things you pointed out. There are a couple more things I need to get further feedback from you.

1) You asked me to remove "copyright to apple". What should I put there? Do I just need to remove the copyright line and keep other comments intact?

2) As to calling ThreadSpecificThreadExit, I do not think making the cleanup call from WTF thread is enough. This approach does not take care of the TLS cleanup in the main thread and other threads not created by WTF::createThread. I did not have a good solution here either. Which mailing list should I send to for further discussion if needed?

I could only think of the following possible ways:

a) If we could make TLS allocations be self manageable, we do not need to deal with this issue. That is, the caller is responsible to free the TLS allocation when it is not longer needed.

b) Call ThreadSpecificThreadExit in each DllMain() when reason parameter is DLL_THREAD_DETACH or DLL_PROCESS_DETACH.

c) If TLS data is only used in exe, not dll, we can use the hack as described in 
 this article at http://www.codeproject.com/threads/tls.asp.


(In reply to comment #9)
> (In reply to comment #8)
> > The main issue here is that this basically changes JavaScriptCore API in an
> > objectionable way - I don't have a proposal, but this definitely needs to be
> > resolved.
> 
> Oh, in fact, nothing in JavaScriptCore uses ThreadSpecific, so I do have a
> proposal. I think that threads created with WTF::createThread() should just
> clean up when exiting - as it is done in Win32+pthreads case in
> wtfThreadEntryPoint (ThreadingWin.cpp).
> 

[Alexey Proskuryakov]

> 1) You asked me to remove "copyright to apple". What should I put there?

My understanding is that you work for Google - so, it's actually a question for your manager. Normally, it's either a company or an individual that makes the contribution.

> 2) As to calling ThreadSpecificThreadExit, I do not think making the cleanup
> call from WTF thread is enough. This approach does not take care of the TLS
> cleanup in the main thread and other threads not created by WTF::createThread.

We don't care about cleanup on the main thread, similarly to how we intentionally avoid cleaning up most static objects for better performance. Also, ThreadSpecific objects are only used in WebCore, not JavaScriptCore, and all WebCore threads are created with WTF::createThread().

> I did not have a good solution here either. Which mailing list should I send to
> for further discussion if needed?

There's always [e-mail] :)

[Jian Li]

Created attachment 26450 [details]
Proposed Patch

[Jian Li]

I fixed all the issues. Please review again. Thanks a lot.

(In reply to comment #11)
> > 1) You asked me to remove "copyright to apple". What should I put there?
> 
> My understanding is that you work for Google - so, it's actually a question for
> your manager. Normally, it's either a company or an individual that makes the
> contribution.
> 
> > 2) As to calling ThreadSpecificThreadExit, I do not think making the cleanup
> > call from WTF thread is enough. This approach does not take care of the TLS
> > cleanup in the main thread and other threads not created by WTF::createThread.
> 
> We don't care about cleanup on the main thread, similarly to how we
> intentionally avoid cleaning up most static objects for better performance.
> Also, ThreadSpecific objects are only used in WebCore, not JavaScriptCore, and
> all WebCore threads are created with WTF::createThread().
> 
> > I did not have a good solution here either. Which mailing list should I send to
> > for further discussion if needed?
> 
> There's always [e-mail] :)
> 

[Alexey Proskuryakov]

Comment on attachment 26450 [details]
Proposed Patch

> -#if USE(PTHREADS) || PLATFORM(WIN)
> +#if USE(PTHREADS)
>  // Windows currently doesn't use pthreads for basic threading, but implementing destructor functions is easier
>  // with pthreads, so we use it here.

This comment is no longer true after your change, you should just remove it.

> +// ThreadSpecificThreadExit should be called each time when a thread is detached.

This comment needs to be more specific. What about adding "This is done automatically for threads created with WTF::createThread."?

> +    // Note: any layout change to this struct should also be applied to the duplicated class in ThreadSpecificWin.cpp.

Please don't call it a class, as it's a struct. And I don't think that having a copy is unavoidable - can we find a way to re-use a single definition?

> +    friend void ThreadSpecificThreadExit();

Why is this function a friend of the class?

> +// The maximum number of TLS keys that can be created. For now, this is fixed.
> +const int kMaxTlsKeySize = 256;

Is there a plan to make it not fixed? Please extend the comment to mention why this is desired, or remove the "for now" part, and explain why this is OK.

> +template<typename T>
> +inline ThreadSpecific<T>::ThreadSpecific() : m_key(0)

The initializer should go on a separate line:

inline ThreadSpecific<T>::ThreadSpecific()
    : m_key(0)

> +    m_key = TlsAlloc();
> +    if (m_key == TLS_OUT_OF_INDEXES)
> +        CRASH();
> +
> +    int slot = InterlockedIncrement(&g_tls_key_count) - 1;
> +    if (slot >= kMaxTlsKeySize)
> +        CRASH();
> +    g_tls_keys[slot] = m_key;

You are still re-implementing TLS, but also using the OS-provided functions. With an array of your own, I don't think that you need TlsXXX functions at all, as it only creates possibilities for the two lists to go out of sync.

You can just use an index into g_tls_keys as a key.

> +    // 2) Does not reclaim the key saved in g_tls_keys since we assume the normal usage is always creating ThreadSpecific.

I don't understand this comment.

> +#include "config.h"
> +#include "ThreadSpecific.h"
> +#include <wtf/Noncopyable.h>

There should be an empty line after config.h.

> +#if !USE(PTHREADS)

No need to conditionalize the whole file - platforms that don't need it can just omit it from the build. You can add an #error to make this clearer:

#if !USE(PTHREADS)
#error This file should not be compiled by ports that use Windows native ThreadSpecific implementation
#endif

> +namespace {
> +template<typename T> struct Data : Noncopyable {
> +    T* value;
> +    WTF::ThreadSpecific<T>* owner;
> +};
> +}

Is there really a need to duplicate the structure? Why?

> +void ThreadSpecificThreadExit()
> +{
> +    for (long i = 0; i < g_tls_key_count; i++) {
> +        Data<int>* data = static_cast<Data<int>*>(TlsGetValue(g_tls_keys[i]));
> +        if (data) {
> +            TlsSetValue(data->owner->m_key, 0);
> +            delete data->value;
> +            delete data;
> +        }
> +    }
> +}

This needs to employ the same idiom that pthreads version now does - the value pointer should only be reset after the destructor is called, not before.

There is a significant semantic discrepancy between this cleanup function and what pthreads does. From pthread_key_create manpage:

     An optional destructor function may be associated with each key value.  At thread exit, if a key value
     has a non-NULL destructor pointer, and the thread has a non-NULL value associated with the key, the
     function pointed to is called with the current associated value as its sole argument.  The order of
     destructor calls is unspecified if more than one destructor exists for a thread when it exits.

     If, after all the destructors have been called for all non-NULL values with associated destructors,
     there are still some non-NULL values with associated destructors, then the process is repeated.  If,
     after at least [PTHREAD_DESTRUCTOR_ITERATIONS] iterations of destructor calls for outstanding non-NULL
     values, there are still some non-NULL values with associated destructors, the implementation stops
     calling destructors.

Your implementation doesn't perform the second part (repetition). I don't think we rely on it anywhere in WebKit now, but this difference should be at least documented in blazing detail, and at best eliminated.

This is getting closer to landable shape, but still needs improvement, r- for now.

[Alexey Proskuryakov]

(In reply to comment #14)
> You are still re-implementing TLS, but also using the OS-provided functions.
> With an array of your own, I don't think that you need TlsXXX functions at all,

Please ignore this comment, it's nonsense.

[Jian Li]

Created attachment 26483 [details]
Proposed Patch

[Jian Li]

Created attachment 26484 [details]
Proposed Patch

[Jian Li]

I've fixed all issues. Please take a look at my latest patch 26484 and ignore the 5th patch. Please also see some comments embedded below. Thanks.

(In reply to comment #14)
> (From update of attachment 26450 [details] [review])
> > -#if USE(PTHREADS) || PLATFORM(WIN)
> > +#if USE(PTHREADS)
> >  // Windows currently doesn't use pthreads for basic threading, but implementing destructor functions is easier
> >  // with pthreads, so we use it here.
> 
> This comment is no longer true after your change, you should just remove it.
> 
> > +// ThreadSpecificThreadExit should be called each time when a thread is detached.
> 
> This comment needs to be more specific. What about adding "This is done
> automatically for threads created with WTF::createThread."?
> 
> > +    // Note: any layout change to this struct should also be applied to the duplicated class in ThreadSpecificWin.cpp.
> 
> Please don't call it a class, as it's a struct. And I don't think that having a
> copy is unavoidable - can we find a way to re-use a single definition?
> 
> > +    friend void ThreadSpecificThreadExit();
> 
> Why is this function a friend of the class?

Removed.
> 
> > +// The maximum number of TLS keys that can be created. For now, this is fixed.
> > +const int kMaxTlsKeySize = 256;
> 
> Is there a plan to make it not fixed? Please extend the comment to mention why
> this is desired, or remove the "for now" part, and explain why this is OK.
> 
> > +template<typename T>
> > +inline ThreadSpecific<T>::ThreadSpecific() : m_key(0)
> 
> The initializer should go on a separate line:
> 
> inline ThreadSpecific<T>::ThreadSpecific()
>     : m_key(0)
> 
> > +    m_key = TlsAlloc();
> > +    if (m_key == TLS_OUT_OF_INDEXES)
> > +        CRASH();
> > +
> > +    int slot = InterlockedIncrement(&g_tls_key_count) - 1;
> > +    if (slot >= kMaxTlsKeySize)
> > +        CRASH();
> > +    g_tls_keys[slot] = m_key;
> 
> You are still re-implementing TLS, but also using the OS-provided functions.
> With an array of your own, I don't think that you need TlsXXX functions at all,
> as it only creates possibilities for the two lists to go out of sync.
> 
> You can just use an index into g_tls_keys as a key.
> 
> > +    // 2) Does not reclaim the key saved in g_tls_keys since we assume the normal usage is always creating ThreadSpecific.
> 
> I don't understand this comment.

I removed the comment here. Please see my comment above for more details.
> 
> > +#include "config.h"
> > +#include "ThreadSpecific.h"
> > +#include <wtf/Noncopyable.h>
> 
> There should be an empty line after config.h.
> 
> > +#if !USE(PTHREADS)
> 
> No need to conditionalize the whole file - platforms that don't need it can
> just omit it from the build. You can add an #error to make this clearer:
> 
> #if !USE(PTHREADS)
> #error This file should not be compiled by ports that use Windows native
> ThreadSpecific implementation
> #endif
> 
> > +namespace {
> > +template<typename T> struct Data : Noncopyable {
> > +    T* value;
> > +    WTF::ThreadSpecific<T>* owner;
> > +};
> > +}
> 
> Is there really a need to duplicate the structure? Why?

Changed declaration of Data from private to public in order to remove the duplicate definition.
> 
> > +void ThreadSpecificThreadExit()
> > +{
> > +    for (long i = 0; i < g_tls_key_count; i++) {
> > +        Data<int>* data = static_cast<Data<int>*>(TlsGetValue(g_tls_keys[i]));
> > +        if (data) {
> > +            TlsSetValue(data->owner->m_key, 0);
> > +            delete data->value;
> > +            delete data;
> > +        }
> > +    }
> > +}
> 
> This needs to employ the same idiom that pthreads version now does - the value
> pointer should only be reset after the destructor is called, not before.
> 
> There is a significant semantic discrepancy between this cleanup function and
> what pthreads does. From pthread_key_create manpage:
> 
>      An optional destructor function may be associated with each key value.  At
> thread exit, if a key value
>      has a non-NULL destructor pointer, and the thread has a non-NULL value
> associated with the key, the
>      function pointed to is called with the current associated value as its
> sole argument.  The order of
>      destructor calls is unspecified if more than one destructor exists for a
> thread when it exits.
> 
>      If, after all the destructors have been called for all non-NULL values
> with associated destructors,
>      there are still some non-NULL values with associated destructors, then the
> process is repeated.  If,
>      after at least [PTHREAD_DESTRUCTOR_ITERATIONS] iterations of destructor
> calls for outstanding non-NULL
>      values, there are still some non-NULL values with associated destructors,
> the implementation stops
>      calling destructors.
> 
> Your implementation doesn't perform the second part (repetition). I don't think
> we rely on it anywhere in WebKit now, but this difference should be at least
> documented in blazing detail, and at best eliminated.

I think we do not need to add this comment because pthread_setspecific is called in destroy() destructor to set the TLS value to NULL and thus repeated call of destructor is not likely.
> 
> 
> This is getting closer to landable shape, but still needs improvement, r- for
> now.
> 

[Alexey Proskuryakov]

Comment on attachment 26484 [details]
Proposed Patch

> -#if USE(PTHREADS) || PLATFORM(WIN)
> -// Windows currently doesn't use pthreads for basic threading, but implementing destructor functions is easier
> -// with pthreads, so we use it here.
> -#include <pthread.h>
> +#if !USE(PTHREADS) && PLATFORM(WIN_OS)
> +#include <windows.h>
>  #endif

Why did you remove the include of pthread.h? I only requested removing the comment (Windows no longer uses pthreads for ThreadSpecific, so explaining why we do that makes no sense), not the whole section.

> +extern void ThreadSpecificThreadExit();

There is no need for "extern" here - function declarations are always extern.

+        ThreadSpecific<int>::Data* data = static_cast<ThreadSpecific<int>::Data*>(TlsGetValue(g_tls_keys[i]));

As mentioned earlier, this needs a comment explaining why it is OK to always use ThreadSpecific<int>.

+        if (data) {
+            data->destructor(data);
+        }

Please omit braces around this single-line statement.

> I think we do not need to add this comment because pthread_setspecific is
> called in destroy() destructor to set the TLS value to NULL and thus repeated
> call of destructor is not likely.

Actually, this is not sufficient, because a different thread specific value can become non-NULL: we only reset the one being destroyed at the moment.

+#if !USE(PTHREADS)
+    void (*destructor)(void*);
+#endif

Oh, that's a great fix, I missed this problem in my review!

>> > +    friend void ThreadSpecificThreadExit();
>> 
>> Why is this function a friend of the class?
>
> Removed.
<...>
> Changed declaration of Data from private to public in order to remove the
> duplicate definition.

Perhaps I do not understand what problem you are solving here, but why not keep Data private, while keeping the friend declaration? Would the duplicate you remove be needed in that case?

This is getting close, but I think it could use one more round of review - at least because of the removed pthread.h include, which would break the build on some platforms!

[Jian Li]

Created attachment 26502 [details]
Proposed Patch

[Jian Li]

All fixed. Thanks.

(In reply to comment #19)
> (From update of attachment 26484 [details] [review])
> > -#if USE(PTHREADS) || PLATFORM(WIN)
> > -// Windows currently doesn't use pthreads for basic threading, but implementing destructor functions is easier
> > -// with pthreads, so we use it here.
> > -#include <pthread.h>
> > +#if !USE(PTHREADS) && PLATFORM(WIN_OS)
> > +#include <windows.h>
> >  #endif
> 
> Why did you remove the include of pthread.h? I only requested removing the
> comment (Windows no longer uses pthreads for ThreadSpecific, so explaining why
> we do that makes no sense), not the whole section.
> 
Sorry, I accidentally removed this. I put it back.
> > +extern void ThreadSpecificThreadExit();
> 
> There is no need for "extern" here - function declarations are always extern.
> 
Removed.
> +        ThreadSpecific<int>::Data* data =
> static_cast<ThreadSpecific<int>::Data*>(TlsGetValue(g_tls_keys[i]));
> 
> As mentioned earlier, this needs a comment explaining why it is OK to always
> use ThreadSpecific<int>.
> 
Added comment.
> +        if (data) {
> +            data->destructor(data);
> +        }
> 
> Please omit braces around this single-line statement.
> 
Removed.
> > I think we do not need to add this comment because pthread_setspecific is
> > called in destroy() destructor to set the TLS value to NULL and thus repeated
> > call of destructor is not likely.
> 
> Actually, this is not sufficient, because a different thread specific value can
> become non-NULL: we only reset the one being destroyed at the moment.
> 
Added the comment at the beginning of the file.
> +#if !USE(PTHREADS)
> +    void (*destructor)(void*);
> +#endif
> 
> Oh, that's a great fix, I missed this problem in my review!
> 
> >> > +    friend void ThreadSpecificThreadExit();
> >> 
> >> Why is this function a friend of the class?
> >
> > Removed.
> <...>
> > Changed declaration of Data from private to public in order to remove the
> > duplicate definition.
> 
> Perhaps I do not understand what problem you are solving here, but why not keep
> Data private, while keeping the friend declaration? Would the duplicate you
> remove be needed in that case?
> 
Originally I added friend declaration in order to let ThreadSpecificThreadExit to access some private data member in ThreadSpecific<>. It is not needed now. However, just as you suggested, this friend declaration can be used to keep Data private. So I put it back.
> This is getting close, but I think it could use one more round of review - at
> least because of the removed pthread.h include, which would break the build on
> some platforms!
> 

[Alexey Proskuryakov]

Comment on attachment 26502 [details]
Proposed Patch

> + * WebKit the repeated call bahavior is utilized.

Typo: "bahavior".

> +#include "config.h"
> +
> +#include "ThreadSpecific.h"

The coding style asks for an empty line after ThreadSpecific.h, not after config.h.

> +		// The layout of ThreadSpecific<T>::Data does not depend on T. So we are safe to do the static cast to ThreadSpecific<int> in order to access its data member.

Please use spaces for indenting, not tabs.

r=me, I'm going to fix these minor details before landing.

[Alexey Proskuryakov]

Committed r39708. While waiting for build, I forgot to fix the nitpicks other than the tabs, butt hey are really minor.

[Nico Weber]

Comment on attachment 26502 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=26502&action=review

> JavaScriptCore/wtf/ThreadSpecific.h:152
> +    // Does not invoke destructor functions. They will be called from ThreadSpecificThreadExit when the thread is detached.

You probably don't care since it's now 9 years after you wrote this code, but this dtor is never called. ThreadSpecificThreadExit calls the destructor function, but that's set to &ThreadSpecific<T>::destroy, and that only calls data->value->~T(), not data->owner->~ThreadSpecific(). From what I can tell, this is still the case in webkit trunk today.