Bug 132800 - Clean up CrossThreadTask
Summary: Clean up CrossThreadTask
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on: 132798 132799
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-10 23:28 PDT by Zan Dobersek
Modified: 2014-05-12 13:17 PDT (History)
1 user (show)

See Also:


Attachments
Patch (58.48 KB, patch)
2014-05-10 23:36 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (58.43 KB, patch)
2014-05-12 03:32 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-05-10 23:28:15 PDT
Clean up CrossThreadTask
Comment 1 Zan Dobersek 2014-05-10 23:36:30 PDT
Created attachment 231253 [details]
Patch
Comment 2 Darin Adler 2014-05-11 11:12:32 PDT
Comment on attachment 231253 [details]
Patch

This is a fine simplification, but I think we should go even further. I don’t think we want to use CrossThreadCopier. Instead we should have lambdas for each of these and copy the arguments explicitly at the call site. Need a new patch that compiles, though. EWS shows this failing to compile on most platforms.
Comment 3 Zan Dobersek 2014-05-11 14:00:39 PDT
(In reply to comment #2)
> (From update of attachment 231253 [details])
> This is a fine simplification, but I think we should go even further. I don’t think we want to use CrossThreadCopier. Instead we should have lambdas for each of these and copy the arguments explicitly at the call site. Need a new patch that compiles, though. EWS shows this failing to compile on most platforms.

I'm all for creating manual cross-thread copies at call sites, but with C++11 lambdas this incurs an additional copy for objects of types like String. Cross-thread copies for these are created through str.isolatedCopy(), but the return value has to be copied once more when it's being value-captured by the lambda function.

C++14 fixes that by providing support for init-capture -- http://isocpp.org/files/papers/N3648.html

I guess this won't be perfect until we make C++14 mandatory. Until then we can either stick to CrossThreadTask and CrossThreadCopier, or suffer through the additional copying overhead.
Comment 4 Darin Adler 2014-05-11 23:24:13 PDT
(In reply to comment #3)
> I'm all for creating manual cross-thread copies at call sites, but with C++11 lambdas this incurs an additional copy for objects of types like String.

Sure, but lets keep in mind that “copy” here just means a ref/deref pair. I think that’s an OK cost.
Comment 5 Zan Dobersek 2014-05-12 03:25:53 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > I'm all for creating manual cross-thread copies at call sites, but with C++11 lambdas this incurs an additional copy for objects of types like String.
> 
> Sure, but lets keep in mind that “copy” here just means a ref/deref pair. I think that’s an OK cost.

OK, I'll give it a look.
Comment 6 Zan Dobersek 2014-05-12 03:32:17 PDT
Created attachment 231284 [details]
Patch
Comment 7 Zan Dobersek 2014-05-12 13:17:11 PDT
Comment on attachment 231284 [details]
Patch

Clearing flags on attachment: 231284

Committed r168640: <http://trac.webkit.org/changeset/168640>
Comment 8 Zan Dobersek 2014-05-12 13:17:18 PDT
All reviewed patches have been landed.  Closing bug.