Bug 57434

Created attachment 87487 [details] Patch - Implement scheduleWorkAfterDelay() using a timeout source - Use an idle source instead of a timeout one with 0 for scheduleWork() - Don't use a lock for g_source_attach() since it uses its own mutex internally - Don't leak the GSource, g_source_attach() increments the source reference counter - Use a GSocket instead of GIOChannel - Factor out performWork() and performWorkOnce() to avoid duplicated code

Comment on attachment 87487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87487&action=review Great patch. I'd r+ it except for the missing ChangeLog information. > Source/WebKit2/ChangeLog:19 > + * Platform/gtk/WorkQueueGtk.cpp: > + (WorkQueue::EventSource::executeEventSource): > + (WorkQueue::EventSource::performWorkOnce): > + (WorkQueue::EventSource::performWork): > + (WorkQueue::workQueueThreadBody): > + (WorkQueue::registerEventSourceHandler): > + (WorkQueue::unregisterEventSourceHandler): > + (WorkQueue::scheduleWorkOnSource): > + (WorkQueue::scheduleWork): You should fill these out. > Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:139 > { > - GIOChannel* channel = g_io_channel_unix_new(fileDescriptor); > - ASSERT(channel); > - GSource* dispatchSource = g_io_create_watch(channel, static_cast<GIOCondition>(condition)); > + GRefPtr<GSocket> socket = adoptGRef(g_socket_new_from_fd(fileDescriptor, 0)); > + ASSERT(socket); > + GSource* dispatchSource = g_socket_create_source(socket.get(), static_cast<GIOCondition>(condition), 0); For instance here in the ChangeLog it would be useful to explain why a GSocket is preferred to a GIOChannel. > Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:195 > + GRefPtr<GSource> dispatchSource = adoptGRef(g_idle_source_new()); In the past we've had issues with starvation of sources created with g_idle_add. We might want to stick with a timeout source here. Gustavo has more information about this, I believe.

(In reply to comment #2) > (From update of attachment 87487 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87487&action=review > > Great patch. I'd r+ it except for the missing ChangeLog information. > > > Source/WebKit2/ChangeLog:19 > > + * Platform/gtk/WorkQueueGtk.cpp: > > + (WorkQueue::EventSource::executeEventSource): > > + (WorkQueue::EventSource::performWorkOnce): > > + (WorkQueue::EventSource::performWork): > > + (WorkQueue::workQueueThreadBody): > > + (WorkQueue::registerEventSourceHandler): > > + (WorkQueue::unregisterEventSourceHandler): > > + (WorkQueue::scheduleWorkOnSource): > > + (WorkQueue::scheduleWork): > > You should fill these out. Ok, I did it too fast. > > Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:139 > > { > > - GIOChannel* channel = g_io_channel_unix_new(fileDescriptor); > > - ASSERT(channel); > > - GSource* dispatchSource = g_io_create_watch(channel, static_cast<GIOCondition>(condition)); > > + GRefPtr<GSocket> socket = adoptGRef(g_socket_new_from_fd(fileDescriptor, 0)); > > + ASSERT(socket); > > + GSource* dispatchSource = g_socket_create_source(socket.get(), static_cast<GIOCondition>(condition), 0); > > For instance here in the ChangeLog it would be useful to explain why a GSocket is preferred to a GIOChannel. GSocket api is newer and allow passing a cancellable to the source, so that we can cacel the source from any thread. I have a patch on top of this one that implements also void WorkQueue::scheduleWorkOnTermination() and uses a cancellable to cancel the source when the webprocess dies. > > Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:195 > > + GRefPtr<GSource> dispatchSource = adoptGRef(g_idle_source_new()); > > In the past we've had issues with starvation of sources created with g_idle_add. We might want to stick with a timeout source here. Gustavo has more information about this, I believe. I change the priority to avoid issues, I think that a timeout source with 0 it's mostly the same than an idle source with a default priority, but I'm not sure. I haven't seen any problems in my tests.

Created attachment 87694 [details] Updated patch Updated patch that includes the changes needed in WorkQueue.h, that I forgot to add in previous patch, and more verbose changelog as requested by Martin.

Comment on attachment 87694 [details] Updated patch Excellent.

Committed r83278: <http://trac.webkit.org/changeset/83278>