[webkit-reviews] review denied: [Bug 180333] [WTF] Thread::create should have Thread::tryCreate : [Attachment 328294] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 180333: [WTF] Thread::create should have Thread::tryCreate
https://bugs.webkit.org/show_bug.cgi?id=180333

Attachment 328294: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 328294
  --> https://bugs.webkit.org/attachment.cgi?id=328294
Patch

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

The move from RefPtr to Ref is a nice cleanup.

But as for tryCreate, I don’t fully understand this. Do we really think it’s OK
for WebKit to run without some threads that it needs? Maybe we have a
discipline where all threads allocated first are required and then later we can
run in a thread-starved state with some operations failing, but others
continuing to work correctly?

Normally with memory allocation, for example, very few clients are actually
prepared to have a second path if allocation fails. It’s the ones that are
allocating large amounts of data, typically proportional to end user data sets,
and thus those are written to understand the possibility of failure. But small
allocations are assumed to never fail.

I don’t see two separate categories like this for threads. There aren’t cheaper
threads and more costly threads, so we can’t assume that cheaper thread
allocations will succeed after the more costly ones.

It’s OK to go on this path if we have a clear goal in mind, but I am not sure
the goal is clear, at least I don’t understand it.

> Source/WebCore/platform/audio/ReverbConvolver.cpp:66
> -    , m_backgroundThread(0)
> +    , m_backgroundThread(nullptr)
>      , m_wantsToExit(false)
>      , m_moreInputBuffered(false)

Can we initialize these in the class definition?

> Source/WebCore/workers/WorkerThread.cpp:143
> +    m_thread = Thread::tryCreate("WebCore: Worker", [this] {

Why is it OK to change the semantics of WorkerThread::start? Doesn’t it need to
be renamed to tryStart if it can fail? Do callers really handle the case where
it fails?

> Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:222
> +    m_syncThread = Thread::tryCreate("WebCore: IconDatabase", [this] {

Seems that this function is written half-claiming it can handle a failure to
create a thread, but not really. For example, if this open function returns
false, it’s not OK that you then have to call the close function. But we have
already done m_mainThreadNotifier.setActive(true) here, which seems to indicate
I need to call close.

I think that maybe this isn’t quite right when m_syncThread ends up null.

> Source/WebKitLegacy/Storage/StorageThread.cpp:57
> +	   m_thread = Thread::tryCreate("WebCore: LocalStorage", [this] {

Same question: Why is it OK to change the semantics of StorageThread::start?
Doesn’t it need to be renamed to tryStart if it can fail? Do callers really
handle the case where it fails?

Maybe we should not add this thread to activeStorageThreads() if we weren’t
able to create a thread? Again, not sure what the concept is here.

> Source/WebKitLegacy/win/WebKitQuartzCoreAdditions/CVDisplayLink.cpp:-67
> -    ASSERT(m_ioThread);

Why remove this assertion? Doesn’t seem necessary to remove it.