Bug 57434

Summary: [GTK] Implement scheduleWorkAfterDelay() in WorkQueueGtk
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 57540    
Attachments:
Description Flags
Patch
mrobinson: review-
Updated patch none

Description Carlos Garcia Campos 2011-03-30 02:18:36 PDT
WorkQueue::scheduleWorkAfterDelay() is currently unimplemented, the code would be similar to WorkQueue::scheduleWork().
Comment 1 Carlos Garcia Campos 2011-03-30 02:28:25 PDT
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 2 Martin Robinson 2011-03-30 09:51:10 PDT
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.
Comment 3 Carlos Garcia Campos 2011-03-30 09:58:58 PDT
(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.
Comment 4 Carlos Garcia Campos 2011-03-31 04:10:45 PDT
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 5 Martin Robinson 2011-03-31 09:40:45 PDT
Comment on attachment 87694 [details]
Updated patch

Excellent.
Comment 6 Carlos Garcia Campos 2011-04-08 03:17:24 PDT
Committed r83278: <http://trac.webkit.org/changeset/83278>