Bug 231418 – Clearly distinguish serial from concurrent WorkQueue

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Jean-Yves Avenard [:jya]]

Currently we have a unique WorkQueue object, which can be configured at construction time to be either a concurrent or a serial one.

A concurrent WorkQueue doesn't guarantee the order in which the queued tasks will run.

That makes for a footgun if a consumer takes a WorkQueue and is expecting it to be a serial one.

We should split the WorkQueue into a ConcurrentWorkQueue object, and make WorkQueue be a serial one.

[Radar WebKit Bug Importer]

<rdar://problem/84021977>

[Jean-Yves Avenard [:jya]]

Created attachment 440702 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 440705 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 440706 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 440708 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 440709 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 440905 [details]
Patch

[Chris Dumez]

Comment on attachment 440905 [details]
Patch

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

EWS is red.

> Source/WTF/wtf/WorkQueue.h:59
> +    enum class Type {

: bool

> Source/WTF/wtf/WorkQueue.h:63
> +    static Ref<WorkQueueBase> create(const char* name, Type, QOS);

Why is this factory function still here? We shouldn't need it.

> Source/WTF/wtf/WorkQueue.h:87
> +class WorkQueue : public WorkQueueBase {

Should be marked as final.

> Source/WTF/wtf/WorkQueue.h:97
> +protected:

Why protected? Seems the constructor should be private. Are there any subclasses of WorkQueue?

> Source/WTF/wtf/WorkQueue.h:115
> +class ConcurrentWorkQueue : public WorkQueueBase {

final

[Jean-Yves Avenard [:jya]]

Comment on attachment 440905 [details]
Patch

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

>> Source/WTF/wtf/WorkQueue.h:59
>> +    enum class Type {
> 
> : bool

when it's existing code like this being moved, I typically keep it as-is.

>> Source/WTF/wtf/WorkQueue.h:87
>> +class WorkQueue : public WorkQueueBase {
> 
> Should be marked as final.

There's SuspendableWorkQueue ; this class and how the IOCache works is the primary reason I went with WebKitBase over the templated version we talked about.

I believe it's WorkQueue people should be inheriting from rather than WorkQueueBase.

>> Source/WTF/wtf/WorkQueue.h:97
>> +protected:
> 
> Why protected? Seems the constructor should be private. Are there any subclasses of WorkQueue?

yes SuspendableWorkQueue

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> Comment on attachment 440905 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440905&action=review
> 
> >> Source/WTF/wtf/WorkQueue.h:59
> >> +    enum class Type {
> > 
> > : bool
> 
> when it's existing code like this being moved, I typically keep it as-is.

Not the WebKit policy though. In WebKit we clean up as we modify existing code since we rarely do pure clean up commits.

> 
> >> Source/WTF/wtf/WorkQueue.h:87
> >> +class WorkQueue : public WorkQueueBase {
> > 
> > Should be marked as final.
> 
> There's SuspendableWorkQueue ; this class and how the IOCache works is the
> primary reason I went with WebKitBase over the templated version we talked
> about.
> 
> I believe it's WorkQueue people should be inheriting from rather than
> WorkQueueBase.
> 
> >> Source/WTF/wtf/WorkQueue.h:97
> >> +protected:
> > 
> > Why protected? Seems the constructor should be private. Are there any subclasses of WorkQueue?
> 
> yes SuspendableWorkQueue

[Jean-Yves Avenard [:jya]]

Created attachment 441010 [details]
Patch

rebase, apply comments

[Chris Dumez]

Comment on attachment 441010 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheIOChannel.h:53
> +    // For safety, it should use a serial WorkQueue (see https://bugs.webkit.org/show_bug.cgi?id=231472)

I still don't understand this comment and I don't think it is accurate. Cocoa uses a concurrent WorkQueue and the code is safe.

[Jean-Yves Avenard [:jya]]

Comment on attachment 441010 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/NetworkCacheIOChannel.h:53
>> +    // For safety, it should use a serial WorkQueue (see https://bugs.webkit.org/show_bug.cgi?id=231472)
> 
> I still don't understand this comment and I don't think it is accurate. Cocoa uses a concurrent WorkQueue and the code is safe.

Yes, this comment is incorrect. Forgot to remove it

[Chris Dumez]

Comment on attachment 441010 [details]
Patch

Ok, r=me but please drop the comment.

[Chris Dumez]

Comment on attachment 441010 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

This is supposed to be above the description, not under.

> Source/WTF/ChangeLog:19
> +        Reviewed by NOBODY (OOPS!).

This is supposed to be above the description, not under.

[Jean-Yves Avenard [:jya]]

Created attachment 441145 [details]
Patch

[Jean-Yves Avenard [:jya]]

grrr another rebasing issue

[Jean-Yves Avenard [:jya]]

Created attachment 441151 [details]
Patch

rebase

[EWS]

Committed r284135 (242955@main): <https://commits.webkit.org/242955@main>

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

[Yusuke Suzuki]

Committed r284137 (242957@main): <https://commits.webkit.org/242957@main>