[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





------- Comment #18 from jianli at chromium.org  2009-01-06 19:07 PDT -------
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 [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.
> 


-- 
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.