Bug 228748 – Suspend WorkQueue of ResourceLoadStatistics and LocalStorage sooner

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Sihui Liu]

...

[Sihui Liu]

Created attachment 434852 [details]
Patch

[Sihui Liu]

Created attachment 434860 [details]
Patch

[Sihui Liu]

Created attachment 434933 [details]
Patch

[Chris Dumez]

Comment on attachment 434933 [details]
Patch

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

I think this is a good idea but I have some comments about the implementation.

> Source/WTF/wtf/SuspendableWorkQueue.cpp:47
> +    // Last suspend function will be the one that is used.

Doesn't seem like a great design.

> Source/WTF/wtf/SuspendableWorkQueue.cpp:54
> +    dispatch([protectedThis = makeRef(*this)] { });

I think this needs a comment to explain that this will suspend the work queue because it is not obvious at first sight. Or maybe even better, call WorkQueue::dispatch() instead of SuspendableWorkQueue::dispatch() and call suspendIfNeeded() explicitly.

> Source/WTF/wtf/SuspendableWorkQueue.cpp:71
> +    WorkQueue::dispatch([this, protectedThis = makeRef(*this), function = WTFMove(function)]() {

protectedThis should not be needed as WorkQueue::dispatch() already takes care of protecting |this|.

'()' is not needed either.

> Source/WTF/wtf/SuspendableWorkQueue.cpp:79
> +    WorkQueue::dispatchAfter(seconds, [this, protectedThis = makeRef(*this), function = WTFMove(function)]() {

same comment about protectedThis and '()'.

> Source/WTF/wtf/SuspendableWorkQueue.cpp:86
> +{

ASSERT(!isMainThread()); ?

> Source/WTF/wtf/SuspendableWorkQueue.cpp:92
> +            if (auto completionHandler = completionHandlers.takeFirst())

We should just iterate over completionHandlers.isEmpty instead of extracting each item one by one.

> Source/WTF/wtf/SuspendableWorkQueue.h:36
> +class SuspendableWorkQueue : public WorkQueue {

final

> Source/WTF/wtf/SuspendableWorkQueue.h:38
> +

Weird blank line.

> Source/WTF/wtf/SuspendableWorkQueue.h:41
> +    WTF_EXPORT_PRIVATE static Ref<SuspendableWorkQueue> create(const char* name, Type = Type::Serial, QOS = QOS::Default);

Type seems like a lie? I don't think your code would be able to suspend a concurrent WorkQueue, only a serial one?

> Source/WTF/wtf/SuspendableWorkQueue.h:43
> +    WTF_EXPORT_PRIVATE void suspend(Function<void()>&&, CompletionHandler<void()>&&);

I don't think we should omit the parameter names here as it is really not obvious what they are.

> Source/WTF/wtf/SuspendableWorkQueue.h:46
> +    WTF_EXPORT_PRIVATE void dispatchAfter(Seconds, Function<void()>&&);

Shouldn't this be marked as virtual in the base class and final here?

> Source/WTF/wtf/SuspendableWorkQueue.h:47
> +

This class seems to completely ignore void dispatchSync(). Why is that OK?

Also, this class should probably make these private:
#if USE(COCOA_EVENT_LOOP)
    dispatch_queue_t dispatchQueue() const { return m_dispatchQueue.get(); }
#else
    RunLoop& runLoop() const { return *m_runLoop; }
#endif

Wouldn't be perfect as the caller could have a pointer to the base class and still get the underlying dispatch_queue but still better than nothing.

> Source/WTF/wtf/SuspendableWorkQueue.h:50
> +    void invokeAllSuspensionCompletionHandlers();

WTF_REQUIRES_LOCK(m_suspensionLock)

> Source/WTF/wtf/SuspendableWorkQueue.h:55
> +    bool m_shouldSuspend WTF_GUARDED_BY_LOCK(m_suspensionLock) { false };

I think m_shouldSuspend is a confusing name since it currently means "will-suspend" and "is-suspended". If we don't need to distinguish the 2 cases, I'd suggest a m_isSuspended naming instead. (or m_isSuspendedOrAboutToBe)

> Source/WTF/wtf/SuspendableWorkQueue.h:56
> +    Function<void()> m_suspendFunction WTF_GUARDED_BY_LOCK(m_suspensionLock);

I am confused by the fact that there can be multiple completion handlers but a single suspendFunction. This means that every time you call suspend(), you overwrite the previous caller's suspend function.
If the suspend function is constant (doesn't change), then maybe it should be passed to the constructor instead?

> Source/WTF/wtf/SuspendableWorkQueue.h:57
> +    Deque<CompletionHandler<void()>> m_suspensionCompletionHandlers;

WTF_GUARDED_BY_LOCK(m_suspensionLock) ?

We add completion handlers from the main thread and remove them from the background thread it seems.

Also it seems like it should be a Vector, not a Deque? You only append and iterate.

> Source/WTF/wtf/SuspendableWorkQueue.h:60
> +} // namespace WTF

Please add the following statement after this:
using WTF::SuspendableWorkQueue;

Then you won't need WTF:: every time you use it and this is the usual pattern in WTF.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:350
> +ResourceLoadStatisticsDatabaseStore::ResourceLoadStatisticsDatabaseStore(WebResourceLoadStatisticsStore& store, WTF::SuspendableWorkQueue& workQueue, ShouldIncludeLocalhost shouldIncludeLocalhost, const String& storageDirectoryPath, PAL::SessionID sessionID)

WTF:: shouldn't be needed.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:148
> +    static NeverDestroyed<Ref<WTF::SuspendableWorkQueue>> queue(WTF::SuspendableWorkQueue::create("WebResourceLoadStatisticsStore Process Data Queue",  WorkQueue::Type::Serial, WorkQueue::QOS::Utility));

nit: redundant space before WorkQueue::Type::Serial

[Chris Dumez]

Comment on attachment 434933 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:180
> +    m_queue->suspend([protectedThis = makeRef(*this)] {

This seems like a retain-cycle and will likely cause leaks. |this| refs m_queue and m_queue refs |this| via m_suspendFunction.

[Sihui Liu]

Comment on attachment 434933 [details]
Patch

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

>> Source/WTF/wtf/SuspendableWorkQueue.cpp:47
>> +    // Last suspend function will be the one that is used.
> 
> Doesn't seem like a great design.

Is it better to store all suspend functions and invoke at suspension?

>> Source/WTF/wtf/SuspendableWorkQueue.cpp:54
>> +    dispatch([protectedThis = makeRef(*this)] { });
> 
> I think this needs a comment to explain that this will suspend the work queue because it is not obvious at first sight. Or maybe even better, call WorkQueue::dispatch() instead of SuspendableWorkQueue::dispatch() and call suspendIfNeeded() explicitly.

Sure, will update.

>> Source/WTF/wtf/SuspendableWorkQueue.cpp:71
>> +    WorkQueue::dispatch([this, protectedThis = makeRef(*this), function = WTFMove(function)]() {
> 
> protectedThis should not be needed as WorkQueue::dispatch() already takes care of protecting |this|.
> 
> '()' is not needed either.

Will remove.

>> Source/WTF/wtf/SuspendableWorkQueue.cpp:79
>> +    WorkQueue::dispatchAfter(seconds, [this, protectedThis = makeRef(*this), function = WTFMove(function)]() {
> 
> same comment about protectedThis and '()'.

Will remove.

>> Source/WTF/wtf/SuspendableWorkQueue.cpp:86
>> +{
> 
> ASSERT(!isMainThread()); ?

Will add.

>> Source/WTF/wtf/SuspendableWorkQueue.cpp:92
>> +            if (auto completionHandler = completionHandlers.takeFirst())
> 
> We should just iterate over completionHandlers.isEmpty instead of extracting each item one by one.

Okay.

>> Source/WTF/wtf/SuspendableWorkQueue.h:36
>> +class SuspendableWorkQueue : public WorkQueue {
> 
> final

Will add.

>> Source/WTF/wtf/SuspendableWorkQueue.h:38
>> +
> 
> Weird blank line.

Will remove.

>> Source/WTF/wtf/SuspendableWorkQueue.h:41
>> +    WTF_EXPORT_PRIVATE static Ref<SuspendableWorkQueue> create(const char* name, Type = Type::Serial, QOS = QOS::Default);
> 
> Type seems like a lie? I don't think your code would be able to suspend a concurrent WorkQueue, only a serial one?

Right, will make type Serial the default and only option.

>> Source/WTF/wtf/SuspendableWorkQueue.h:43
>> +    WTF_EXPORT_PRIVATE void suspend(Function<void()>&&, CompletionHandler<void()>&&);
> 
> I don't think we should omit the parameter names here as it is really not obvious what they are.

Will add.

>> Source/WTF/wtf/SuspendableWorkQueue.h:46
>> +    WTF_EXPORT_PRIVATE void dispatchAfter(Seconds, Function<void()>&&);
> 
> Shouldn't this be marked as virtual in the base class and final here?

Okay.

>> Source/WTF/wtf/SuspendableWorkQueue.h:47
>> +
> 
> This class seems to completely ignore void dispatchSync(). Why is that OK?
> 
> Also, this class should probably make these private:
> #if USE(COCOA_EVENT_LOOP)
>     dispatch_queue_t dispatchQueue() const { return m_dispatchQueue.get(); }
> #else
>     RunLoop& runLoop() const { return *m_runLoop; }
> #endif
> 
> Wouldn't be perfect as the caller could have a pointer to the base class and still get the underlying dispatch_queue but still better than nothing.

dispatchSync will be the same as WorkQueue::dispatchSync() if suspend() is not called. If suspend() is called, then client should not use dispatchSync as it will block the thread. I guess we can add assert in SuspendableWorkQueue::dispatchSync to ensure that.

Will make the functions private.

>> Source/WTF/wtf/SuspendableWorkQueue.h:50
>> +    void invokeAllSuspensionCompletionHandlers();
> 
> WTF_REQUIRES_LOCK(m_suspensionLock)

Okay.

>> Source/WTF/wtf/SuspendableWorkQueue.h:55
>> +    bool m_shouldSuspend WTF_GUARDED_BY_LOCK(m_suspensionLock) { false };
> 
> I think m_shouldSuspend is a confusing name since it currently means "will-suspend" and "is-suspended". If we don't need to distinguish the 2 cases, I'd suggest a m_isSuspended naming instead. (or m_isSuspendedOrAboutToBe)

Okay.

>> Source/WTF/wtf/SuspendableWorkQueue.h:56
>> +    Function<void()> m_suspendFunction WTF_GUARDED_BY_LOCK(m_suspensionLock);
> 
> I am confused by the fact that there can be multiple completion handlers but a single suspendFunction. This means that every time you call suspend(), you overwrite the previous caller's suspend function.
> If the suspend function is constant (doesn't change), then maybe it should be passed to the constructor instead?

The suspend function may hold ref to the owner of WorkQueue, so it's either set m_suspendFunction at suspend(), or set it in constructor and clear it at some time later.

>> Source/WTF/wtf/SuspendableWorkQueue.h:57
>> +    Deque<CompletionHandler<void()>> m_suspensionCompletionHandlers;
> 
> WTF_GUARDED_BY_LOCK(m_suspensionLock) ?
> 
> We add completion handlers from the main thread and remove them from the background thread it seems.
> 
> Also it seems like it should be a Vector, not a Deque? You only append and iterate.

Sure.

>> Source/WTF/wtf/SuspendableWorkQueue.h:60
>> +} // namespace WTF
> 
> Please add the following statement after this:
> using WTF::SuspendableWorkQueue;
> 
> Then you won't need WTF:: every time you use it and this is the usual pattern in WTF.

Okay.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:350
>> +ResourceLoadStatisticsDatabaseStore::ResourceLoadStatisticsDatabaseStore(WebResourceLoadStatisticsStore& store, WTF::SuspendableWorkQueue& workQueue, ShouldIncludeLocalhost shouldIncludeLocalhost, const String& storageDirectoryPath, PAL::SessionID sessionID)
> 
> WTF:: shouldn't be needed.

Will remove.

>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:148
>> +    static NeverDestroyed<Ref<WTF::SuspendableWorkQueue>> queue(WTF::SuspendableWorkQueue::create("WebResourceLoadStatisticsStore Process Data Queue",  WorkQueue::Type::Serial, WorkQueue::QOS::Utility));
> 
> nit: redundant space before WorkQueue::Type::Serial

Will remove.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:180
>> +    m_queue->suspend([protectedThis = makeRef(*this)] {
> 
> This seems like a retain-cycle and will likely cause leaks. |this| refs m_queue and m_queue refs |this| via m_suspendFunction.

Yes, I should clear suspend function when clearing suspension completion handlers.

[Sihui Liu]

Created attachment 435022 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81626714>

[Chris Dumez]

Comment on attachment 435022 [details]
Patch

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

> Source/WTF/wtf/SuspendableWorkQueue.cpp:127
> +        m_suspendFunction();

Why aren't we nulling out the suspend function right after calling it? e.g.
if (auto suspendFunction = std::exchange(m_suspendFunction, {}))
    suspendFunction();

It looks like we could simplify this function a bit:
{
    Locker suspensionLocker { m_suspensionLock };
    auto suspendFunction = std::exchange(m_suspendFunction, {});
    if (m_isOrWillBeSuspended)
        suspendFunction();
    invokeAllSuspensionCompletionHandlers();
    while (m_isOrWillBeSuspended)
        m_suspensionCondition.wait(m_suspensionLock);
}

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageNamespace.h:36
> +class SuspendableWorkQueue;

You should consider adding this forward declaration to wtf/Forward.h with the adequate `using` statement so that you don't have to use `WTF::` prefix.

[Sihui Liu]

Created attachment 435358 [details]
Patch

[Chris Dumez]

Comment on attachment 435358 [details]
Patch

r=me

[EWS]

Committed r280934 (240451@main): <https://commits.webkit.org/240451@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435358 [details].