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.
<rdar://problem/84021977>
Created attachment 440702 [details] Patch
Created attachment 440705 [details] Patch
Created attachment 440706 [details] Patch
Created attachment 440708 [details] Patch
Created attachment 440709 [details] Patch
Created attachment 440905 [details] Patch
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
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
(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
Created attachment 441010 [details] Patch rebase, apply comments
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.
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
Comment on attachment 441010 [details] Patch Ok, r=me but please drop the comment.
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.
Created attachment 441145 [details] Patch
grrr another rebasing issue
Created attachment 441151 [details] Patch rebase
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].
Committed r284137 (242957@main): <https://commits.webkit.org/242957@main>