Bug 145797 - [SOUP] Network Cache: run the IO completion handler in the given queue instead of the whole operation
Summary: [SOUP] Network Cache: run the IO completion handler in the given queue instea...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, Soup
Depends on:
Blocks:
 
Reported: 2015-06-09 05:42 PDT by Carlos Garcia Campos
Modified: 2015-06-09 10:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.86 KB, patch)
2015-06-09 05:44 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (11.09 KB, patch)
2015-06-09 08:31 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-06-09 05:42:57 PDT
I misunderstood what the WorkQueue parameter meant in the IO channel operations. It's the queue where the completion handler should be run, not the whole operation. Since our operations are already non-blocking, we can just run the read/writes in the main thread, and schedule the completion handler in the given work queue when the operation finishes.
Comment 1 Carlos Garcia Campos 2015-06-09 05:44:54 PDT
Created attachment 254561 [details]
Patch
Comment 2 WebKit Commit Bot 2015-06-09 05:46:17 PDT
Attachment 254561 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:156:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:178:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:265:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2015-06-09 06:09:29 PDT
Comment on attachment 254561 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119
> +            auto asyncData = std::unique_ptr<ReadAsyncData>(asyncDataPtr);

This can be just

    std::unique_ptr<ReadAsyncData> asyncData(asyncDataPtr);

Same for the rest of such statements.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:188
> +            Data data;
> +            completionHandler(data, -1);

You could probably just construct the Data object in the completionHandler call:

    completionHandler(Data(), -1);

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:191
> +        g_cond_wait(&condition, &mutex);

Can this function be called on the main thread, with a null queue?

That would schedule the callback in the main context and continue to block the main thread in g_cond_wait(), with the callback never invoked.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:223
>      g_cond_wait(&condition, &mutex);

Same problem here.
Comment 4 Carlos Garcia Campos 2015-06-09 08:24:22 PDT
(In reply to comment #3)
> Comment on attachment 254561 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254561&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119
> > +            auto asyncData = std::unique_ptr<ReadAsyncData>(asyncDataPtr);
> 
> This can be just
> 
>     std::unique_ptr<ReadAsyncData> asyncData(asyncDataPtr);
> 
> Same for the rest of such statements.

Ok, no much difference anyway.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:188
> > +            Data data;
> > +            completionHandler(data, -1);
> 
> You could probably just construct the Data object in the completionHandler
> call:
> 
>     completionHandler(Data(), -1);

No, that doesn't build.

no known conversion for argument 1 from ‘WebKit::NetworkCache::Data’ to ‘WebKit::NetworkCache::Data&’

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:191
> > +        g_cond_wait(&condition, &mutex);
> 
> Can this function be called on the main thread, with a null queue?

No this, is always called from a thread. I'll add an assert to ensure it.

> That would schedule the callback in the main context and continue to block
> the main thread in g_cond_wait(), with the callback never invoked.
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:223
> >      g_cond_wait(&condition, &mutex);
> 
> Same problem here.

Yes, we definitely don't want to block the main thread ever.
Comment 5 Carlos Garcia Campos 2015-06-09 08:31:17 PDT
Created attachment 254570 [details]
Updated patch
Comment 6 WebKit Commit Bot 2015-06-09 08:34:19 PDT
Attachment 254570 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:157:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:179:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:268:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Zan Dobersek 2015-06-09 08:47:57 PDT
Comment on attachment 254570 [details]
Updated patch

Looks good.
Comment 8 Carlos Garcia Campos 2015-06-09 10:08:17 PDT
Committed r185365: <http://trac.webkit.org/changeset/185365>