[webkit-reviews] review granted: [Bug 175187] [WTF] ThreadSpecific should not introduce additional indirection : [Attachment 317775] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 175187: [WTF] ThreadSpecific should not introduce additional indirection
https://bugs.webkit.org/show_bug.cgi?id=175187

Attachment 317775: Patch

https://bugs.webkit.org/attachment.cgi?id=317775&action=review




--- Comment #15 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 317775
  --> https://bugs.webkit.org/attachment.cgi?id=317775
Patch

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

r=me with suggestions.

> Source/WTF/ChangeLog:15
> +	   This patch adds storage to Data and initializes T in Data.
> +	   And we drop ThreadSpecific::replace support, which is only used
> +	   by Web thread. Web thread should use
ThreadSpecific<std::unique_ptr<T>> instead.

I suggest rephrasing this as:

This patch adds storage in Data in order to embed the instance of T. The
constructor for Data will invoke the constructor for T on the embedded storage.
We also drop ThreadSpecific::replace which is only used by the web thread to
set its thread specific ThreadGlobalData to the one shared from the main
thread. The existing implementation relies on the main thread and the web
thread never exiting in order for the shared ThreadGlobalData to stay alive. We
can achieve the same semantics by using a ThreadSpecific<std::unique_ptr<T>> to
hold the ThreadGlobalData instance instead.

> Source/WTF/wtf/ThreadSpecific.h:101
> +	   void construct()
> +	   {
> +	       new (NotNull, storagePointer()) T;
> +	   }

You can delete this.  Not needed anymore.  See constructor below.

> Source/WTF/wtf/ThreadSpecific.h:106
> +	   Data(ThreadSpecific<T, canBeGCThread>* owner)
> +	       : owner(owner)
> +	   {
> +	   }

In the body of the constructor, add:
    // Set up thread-specific value's memory pointer before invoking T's
constructor (in case any function it calls
    // needs to access the value) to avoid recursion.
    set(data);
    new (NotNull, storagePointer()) T;

This ensures that T is initialize as part of constructing Data, and is not
reliant on the client to do the work correctly.  It also makes the code more
symmetrical: Data's constructor calls T's constructor, and Data's destructor
calls T's destructor.

> Source/WTF/wtf/ThreadSpecific.h:121
> +    T* setAndConstruct();
> +    void set(Data*);

This is just a suggestion: rename "setAndConstruct" to just "set" (to mirror
"get").  The fact that "set"ting involves allocating Data and constructing T is
just an implementation detail.	Accordingly, rename "set" to "setInTLS" or
"setInternal".	What do you think?

> Source/WTF/wtf/ThreadSpecific.h:290
> +    // Set up thread-specific value's memory pointer before invoking
constructor, in case any function it calls
> +    // needs to access the value, to avoid recursion.

Move this into the Data constructor (see above).

> Source/WTF/wtf/ThreadSpecific.h:295
> +    set(data);
> +    data->construct();

Since we only even create instances of Data that we want to set into TLS, we
can make this cleaner and just do this work in the constructor (see above).

Also add a comment near the "new Data()" line: "// Data will set itself into
TLS."  Or something to that effect.
Alternatively, you can just assert it:
    ASSERT(get() == data->storagePointer());
or add both the assert and the comment.

> Source/WebCore/platform/ThreadGlobalData.cpp:93
> +    // While we store the same pointer to std::unique_ptr, it is ok because
the web thread never finishes.

I suggest rephrasing this as:
// The web thread never finishes, and we expect the main thread to also never
finish. Hence, it is safe to store the same ThreadGlobalData pointer in a
thread specific std::unique_ptr.

I also suggest adding a FIXME comment here with a bug url to change
ThreadGlobalData to be ThreadSafeRefCounted (and staticData to be
ThreadSpecific<RefPtr<ThreadGlobalData>>*) later.  I think that will removes
the need to assume that the main thread never exits, and makes the code less
fragile.  On the other hand, I think there is a lot of expectation that the
"main" thread paired with the web thread is always the main thread (because we
rely on pthread_main_np() to be true for the main thread).  Hence, it is
expected in many places that the main thread never exits, and this issue is
unlikely to ever manifest.  I'll let you decide if you want to do this step or
not.