Bug 175471 - Add in-memory encoder/decoders to WTF
Summary: Add in-memory encoder/decoders to WTF
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-10 20:14 PDT by Brady Eidson
Modified: 2017-12-06 03:30 PST (History)
4 users (show)

See Also:


Attachments
Patch (47.37 KB, patch)
2017-08-10 20:38 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (52.31 KB, patch)
2017-08-10 21:44 PDT, Brady Eidson
sam: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-08-10 20:14:35 PDT
Add in-memory encoder/decoders to WTF

I'll be needing these in WebCore soon, and there's probably other ways we can reuse them going forward.
Comment 1 Brady Eidson 2017-08-10 20:38:11 PDT
Created attachment 317911 [details]
Patch
Comment 2 Build Bot 2017-08-10 20:39:39 PDT
Attachment 317911 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/CMakeLists.txt:278:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WTF/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Sam Weinig 2017-08-10 21:06:28 PDT
What are they going to be used for in WebCore?
Comment 4 Brady Eidson 2017-08-10 21:41:10 PDT
(In reply to Sam Weinig from comment #3)
> What are they going to be used for in WebCore?

ServiceWorker stuff.
Comment 5 Brady Eidson 2017-08-10 21:44:46 PDT
Created attachment 317915 [details]
Patch
Comment 6 Sam Weinig 2017-08-11 05:03:07 PDT
(In reply to Brady Eidson from comment #4)
> (In reply to Sam Weinig from comment #3)
> > What are they going to be used for in WebCore?
> 
> ServiceWorker stuff.

I gathered. But why specifically will the coders be needed in WebCore? Are they for IPC? If so, why not follow the existing pattern of putting the coders in WebKit?
Comment 7 Brady Eidson 2017-08-11 13:49:56 PDT
(In reply to Sam Weinig from comment #6)
> (In reply to Brady Eidson from comment #4)
> > (In reply to Sam Weinig from comment #3)
> > > What are they going to be used for in WebCore?
> > 
> > ServiceWorker stuff.
> 
> I gathered. But why specifically will the coders be needed in WebCore? Are
> they for IPC? If so, why not follow the existing pattern of putting the
> coders in WebKit?

I don't need the convenience of autogeneration, IPC message names and targets, etc., and by keeping everything under WebCore implementation will be lower friction and faster.
Comment 8 Sam Weinig 2017-08-11 16:35:17 PDT
(In reply to Brady Eidson from comment #7)
> (In reply to Sam Weinig from comment #6)
> > (In reply to Brady Eidson from comment #4)
> > > (In reply to Sam Weinig from comment #3)
> > > > What are they going to be used for in WebCore?
> > > 
> > > ServiceWorker stuff.
> > 
> > I gathered. But why specifically will the coders be needed in WebCore? Are
> > they for IPC? If so, why not follow the existing pattern of putting the
> > coders in WebKit?
> 
> I don't need the convenience of autogeneration, IPC message names and
> targets, etc., and by keeping everything under WebCore implementation will
> be lower friction and faster.

So it's not for IPC? What's it for then? Sorry for being slow here, just don't quite get it.
Comment 9 Brady Eidson 2017-08-11 22:42:16 PDT
(In reply to Sam Weinig from comment #8)
> (In reply to Brady Eidson from comment #7)
> > (In reply to Sam Weinig from comment #6)
> > > (In reply to Brady Eidson from comment #4)
> > > > (In reply to Sam Weinig from comment #3)
> > > > > What are they going to be used for in WebCore?
> > > > 
> > > > ServiceWorker stuff.
> > > 
> > > I gathered. But why specifically will the coders be needed in WebCore? Are
> > > they for IPC? If so, why not follow the existing pattern of putting the
> > > coders in WebKit?
> > 
> > I don't need the convenience of autogeneration, IPC message names and
> > targets, etc., and by keeping everything under WebCore implementation will
> > be lower friction and faster.
> 
> So it's not for IPC? What's it for then? Sorry for being slow here, just
> don't quite get it.

It is for IPC. And it is for avoiding having to go up into WebKit and having to rebuild it each time I make a change that should be localized within WebCore.

I am writing patches with small changes in WebCore alone that result in a sub-minute build-link time of WebCore itself.

By relying on "the way things have been" with WebKit2 being the sole arbiter of encoding, my build time skyrockets by having to regenerate messages, rebuild using templated encoders from WebKit2, occasionally rebuild the entire WebKit2 world, stub out new methods, fix link errors, etc etc.

Changes that should be rapid to develop instead slow me down. Significantly.

I can structure significant amounts of code that rely on a single, never-changing message and encoder/decoder pair inside WebKit2, where the meat of the payload can change as needed by solely being encoded/decoded inside WebCore.

This patch is about significantly reducing unnecessary friction in development, and also results in code all being localized to one project instead of spread out over two.
Comment 10 Sam Weinig 2017-08-13 18:30:55 PDT
The alternate coding idiom, that requires less multiframework hopping, that's been used in the past is to add templatizd encode/decode functions to the classes/structs you want participate in coding. For instance, RealtimeMediaSourceSettings uses:

    template<class Encoder> void encode(Encoder&) const;
    template<class Decoder> static bool decode(Decoder&, RealtimeMediaSourceSettings&);

Then, when IPC does happen, whatever message wants to code these can do so without the help of WebCoreArgumentCoders.

The downside of course is that you need to declare them in the header.

I hope either of these alternate strategies.
Comment 11 Brady Eidson 2017-08-14 09:02:35 PDT
(In reply to Sam Weinig from comment #10)
> The alternate coding idiom, that requires less multiframework hopping,
> that's been used in the past is to add templatizd encode/decode functions to
> the classes/structs you want participate in coding. For instance,
> RealtimeMediaSourceSettings uses:
> 
>     template<class Encoder> void encode(Encoder&) const;
>     template<class Decoder> static bool decode(Decoder&,
> RealtimeMediaSourceSettings&);
> 
> Then, when IPC does happen, whatever message wants to code these can do so
> without the help of WebCoreArgumentCoders.

I'm an extreme champion of this technique. I'm intimately familiar with it. If you look at IDB, for example, you'll notice extensive use is made of it there. I've also added it to tons of PAL classes, have moved many WebCoreArgumentCoders from WK to WC, and have converted many other engineers on the project to it.

I know about it.
 
> The downside of course is that you need to declare them in the header.

*AND* go rebuild WK2 after you've made the WebCore change.

> I hope either of these alternate strategies.

AFAIK, you just presented one alternate strategy, not two.

---

Are you r-'ing this patch based on your feedback that there is an existing way to do things, albeit much slower to rapidly develop with?

I would like to move on with development of the actual feature one way or the other.
Comment 12 Sam Weinig 2017-08-14 19:18:00 PDT
(In reply to Brady Eidson from comment #11)
> (In reply to Sam Weinig from comment #10)
> > I hope either of these alternate strategies.
> 
> AFAIK, you just presented one alternate strategy, not two.
> 

Indeed. One. I thought I had mentioned using WebCore argument coders in WebKit as well, but it seems I didn't. Sorry for the confusion.

> ---
> 
> Are you r-'ing this patch based on your feedback that there is an existing
> way to do things, albeit much slower to rapidly develop with?
> 
> I would like to move on with development of the actual feature one way or
> the other.

I don't think it is a good tradeoff to have another set of encoders/decoders and an additional memory allocation / copy for IPC. So, I guess, r-.