[Webkit-unassigned] [Bug 22614] Need to add Win32 implementation of ThreadSpecific.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22614


ap at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26450|review?                     |review-
               Flag|                            |




------- Comment #14 from ap at webkit.org  2009-01-06 02:29 PDT -------
(From update of attachment 26450)
> -#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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.