[webkit-reviews] review granted: [Bug 174081] [WTF] Implement WTF::ThreadGroup : [Attachment 314685] 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 174081: [WTF] Implement WTF::ThreadGroup
https://bugs.webkit.org/show_bug.cgi?id=174081

Attachment 314685: Patch

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




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

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

r=me with issues addressed.

> Source/WTF/wtf/ThreadGroup.cpp:61
> +bool ThreadGroup::add(Thread& thread)
> +{
> +    auto destructionLocker = holdLock(destructionMutex());
> +    auto locker = holdLock(m_lock);
> +    if (!thread.canAddToThreadGroup(destructionLocker))
> +	   return false;
> +    if (m_threads.add(&thread).isNewEntry)
> +	   thread.addToThreadGroup(destructionLocker, *this);
> +    return true;
> +}
> +
> +bool ThreadGroup::addCurrentThread()
> +{
> +    return add(Thread::current());
> +}

Do we really need an API for add() other than addCurrentThread()?  I think this
logic is correct, but if we don't need add(), we can reduce the complexity by
only implementing addCurrentThread(), and removing all the canAddToThreadGroup
logic.	I'm ok with keeping add() too if you think we may need it soon.

Regardless, addCurrentThread() should always succeed.  You can assert that
instead of returning a boolean.

> Source/WTF/wtf/ThreadGroup.h:56
> +    void removeCurrentThread(const AbstractLocker&, Thread&);

Since there are 2 different mutex at play in this code, I recommend naming the
AbstractLocker that should be passed in e.g. destructionMutexLocker (or
destructionLocker to be consistent with how you name it elsewhere ... I prefer
destructionMutexLocker because it explicitly spells out which mutex we want
locked).  I think this documents the code's intent better.

> Source/WTF/wtf/Threading.cpp:152
> +	   m_canAddToThreadGroup = false;

This flag feels redundant because we have m_didExit = true above.  The only
reason we aren't allowed to add the thread is because it's exiting, right?  Or
is there another reason?

One option is you can remove the m_canAddToThreadGroup flag and just have
ThreadGroup::add() check hasExited() instead.  However, will need to hold the
destructionMutex in the  block above which sets m_didExit.  I think
Thread::m_mutex is never held while calling back to higher level functions. 
Hence, you can just hoist the destructionLocker to the top of this function.

> Source/WTF/wtf/Threading.cpp:158
> +    ASSERT_WITH_MESSAGE(Thread::current() == *this, "This function is only
called from the current thread itself.");

nit: maybe rephrase the error message as "This function should only be called
from the current thread itself."  If the assertion fails, then clearly, this
function isn't called from the current thread and contradicts the error
message.

> Source/WTF/wtf/Threading.h:185
> +    bool canAddToThreadGroup(const AbstractLocker&) { return
m_canAddToThreadGroup; }
> +    void addToThreadGroup(const AbstractLocker&, ThreadGroup&);
> +    void removeFromThreadGroup(const AbstractLocker&, ThreadGroup&);

I also recommend naming the AbstractLocker& arguments here as
destructionMutexLocker.

> Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:58
> +	       EXPECT_EQ(threadGroup->add(*thread), true);
> +	       threads.append(thread);

You tested ThreadGroup::add() (adding from another thread) here but didn't test
addCurrentThread() which is actually used in our code base.  I think you should
add a case that tests addCurrentThread().  Maybe factor this test out and
parameterized it to add the thread in the 1 of the 2 ways.