[webkit-reviews] review denied: [Bug 22614] Need to add Win32 implementation of ThreadSpecific. : [Attachment 26450] Proposed Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 22614: Need to add Win32 implementation of ThreadSpecific.
https://bugs.webkit.org/show_bug.cgi?id=22614

Attachment 26450: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=26450&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> -#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.