Re: Mojo, IPC, and Layering

[John Abd-El-Malek]

+chromium-mojo

This is a good question, and I'll add it to the list of things we need to better document in mojo.

Replies inline.

Can you point me at the cl? If there's a missing dependency I can take a look.

On Wed, Apr 6, 2016 at 6:24 PM, Ryan Sleevi <sle...@google.com> wrote:

After landing a change, I happened to notice a build-bot failed because //url/mojo hadn't generated the proper headers. I was surprised to learn //url/mojo was even a thing.

The reason I was surprised is that this means that //net (which depends on //url) now has a quasi-transitive dependency on //mojo 

However, //mojo (via //mojo/services/network) has a dependency on //net. This seems to mean that //net and //mojo have a... complicated relationship. 

Note that mojo/services/network is self contained, i.e. depending on the "core" mojo platform doesn't bring in this code. This directory is actually not used anymore; we were keeping it around to point folks at the mojoms there but we can just point them at the history. I'll send a cl to delete it for now, similar to how we deleted mandoline.

 

I'm hoping someone on this thread can explain how //net and //mojo are "supposed" to relate, as top-level directories - is //net below //mojo in the way that //content is below //chrome, or the inverse, or are we happy with having circular-dependencies between directories, so long as it's in sub-directories - effectively, //mojo is "like" //components, in that some are above //content, and some are below //content?

With old chrome IPC, the layering was much simpler. i.e. roughly

[chrome] [components]

               [content]

[net]           [ipc]          [blink]  

        [base]

With mojo, things are a bit different since now a lot of layers will speak mojo, e.g. Blink or components that sit below content like leveldb or filesystem. This means that it won't work anymore to just put all the shared IPC traits for low-level things like gfx::Rect, URL etc... in content/public and just have components & chrome reuse them that way (since some of the modules that need them are below content). Additionally, the IPC details are hidden with mojo (e.g. routing IDs and IPC::Sender or filters or channels).

As an example of the above, net already has mojoms in one subdirectory.

To solve this problem, for "low-level" classes like gfx::Rect or URL, we're putting the mojo version of traits in those subdirectories. That way components that sit below content can still send GURL or gfx::Rect in mojoms. These traits are in a mojo subdirectory, and are in separate build targets so that depending on src/ui or src/url doesn't bring in a dependency on mojo.

I bring this up because understanding how //net and //mojo relate also helps inform how //net and //ipc should relate.

For old chrome IPC, there are also changes happening now to move traits to "ipc" subdirectories in ui/gfx/ipc, url/ipc, media/ipc, gpu/ipc. While chrome IPC is going to be replaced with mojo, the reason these directories are popping up is just to help the transition. In particular for the Mus work, there are command buffer related IPCs in content that need to be shared with Mus. More background in https://bugs.chromium.org/p/chromium/issues/detail?id=586364.

 

I recently pushed back on a CL that would have added IPC type traits for a //net type inside of //net, as that would have meant that //net now depends on //ipc, and that feels like a layering violation to me.

Can you point us at the cl? Is this related to Mus?

ipc/ doesn't depend on net/, so I'm curious what circular dependency you're thinking of?

 

Will Chan had, in the past, told and taught me that directory-level circular dependencies are, in many ways, similar to class-level circular dependencies, and that both should be avoided, and so that's the background I'm coming from. Just trying to make sense of this, and figure out what the right way to think of these layers as - and whether I was wrong in arguing that //net shouldn't know about //ipc.

These are all valid guidelines and we should continue to stick to them.

Hopefully the above clears things up some of these nuances; happy to chat more tomorrow as well. 

[Darin Fisher]

Yes, circular dependencies between directories are to be avoided, and the plan with mojo subdirs isn't meant to violate that. Ditto for build rules.

It is just about keeping the corresponding mojo support code for a module near the other code for that module. //net maintainers should feel ownership over how their types are reflected over IPC (Mojo IPC and Chrome IPC). The way these significant bits are relegated out to //content is not so great.

I see it as a library providing support code for the ways the library might be used. This is a common thing and can be done in a way that avoids layering violations.

-Darin

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To post to this group, send email to chromi...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CALhVsw0xyA%2BmgRdRZ3AgBuEWLBv9463xvVXKcue3qtM3KJ4n5w%40mail.gmail.com.

[Ryan Sleevi]

collided with jam's reply, so readding the list to my initial reply to Brett.

On Wed, Apr 6, 2016 at 10:01 PM, Ryan Sleevi <sle...@google.com> wrote:

Right, but doesn't having mojom traits implicitly constitute a dependency on mojo? If the type system changes, doesn't that file need to change? Don't the generated bindings depend on the Mojo subsystem?

I mean, the same is true for //url/ipc, which depends on //ipc, which means conceptually there's a dependency between //net -> //url -> //ipc

If it is OK for //url, is it OK for there to be a //net/ipc or a //net//mojo? What about a //base/ipc or //base/mojo, if //base types needed to be serialized?

To be clear, I'm not trying to pick on //url - just trying to understand the rationale behind the decision and the principles I should be using when reviewing //net code - where in the layer graph should we see Mojo and IPC, and where should we put things like traits and serialization for things "low" in the stack?

I had suggested that for the limited case at the time, we duplicate the code between //content and //remoting (which yes, means two serializers), because having //net depend on IPC seems a layering violation (especially when we want to ensure //net works for Cronet, and thus want to be EXTREMELY careful about any DEPS //net OR its child directories take), but perhaps I was wrong? That's a bit more of the context here.

Note I am aware that both //url/ipc and //url/mojo provide separate targets from //url, and thus are not a strict circular dependency. This is more about how we structure and envision directories - should it be OK for, say, an independent //net/foobar target to depend on //content? Because that had also come up in the past, and I've always said a hard no, because //content consumes //net. So that's why I'm trying to understand what principles for IPC and Mojo bits - are they above //base, below //net? Are they siblings to //net (which is what I had assumed)? Or are split directories like //components - where some are above and some are below //content - OK to be more widespread?

On Apr 6, 2016 9:47 PM, "Brett Wilson" <bre...@chromium.org> wrote:
>
> Take a look at the sources and build file in url/mojo (I did, I didn't actually remember what this was for before writing this email).
>
> This is just a shared place to put the traits for sending GURLs over Mojo. It doesn't actually depend on any mojo targets.
>
> Brett

>
> On Wed, Apr 6, 2016 at 6:24 PM, Ryan Sleevi <sle...@google.com> wrote:
>>
>> After landing a change, I happened to notice a build-bot failed because //url/mojo hadn't generated the proper headers. I was surprised to learn //url/mojo was even a thing.
>>
>> The reason I was surprised is that this means that //net (which depends on //url) now has a quasi-transitive dependency on //mojo
>>
>> However, //mojo (via //mojo/services/network) has a dependency on //net. This seems to mean that //net and //mojo have a... complicated relationship. 
>>

>> I'm hoping someone on this thread can explain how //net and //mojo are "supposed" to relate, as top-level directories - is //net below //mojo in the way that //content is below //chrome, or the inverse, or are we happy with having circular-dependencies between directories, so long as it's in sub-directories - effectively, //mojo is "like" //components, in that some are above //content, and some are below //content?
>>

>> I bring this up because understanding how //net and //mojo relate also helps inform how //net and //ipc should relate. I recently pushed back on a CL that would have added IPC type traits for a //net type inside of //net, as that would have meant that //net now depends on //ipc, and that feels like a layering violation to me.

[Ryan Sleevi]

On Wed, Apr 6, 2016 at 9:49 PM, John Abd-El-Malek <j...@chromium.org> wrote:

With old chrome IPC, the layering was much simpler. i.e. roughly

[chrome] [components]

               [content]

[net]           [ipc]          [blink]  

        [base]

Right, this was my understanding.

 

To solve this problem, for "low-level" classes like gfx::Rect or URL, we're putting the mojo version of traits in those subdirectories. That way components that sit below content can still send GURL or gfx::Rect in mojoms. These traits are in a mojo subdirectory, and are in separate build targets so that depending on src/ui or src/url doesn't bring in a dependency on mojo.

OK. I mean, it totally makes sense on one hand, because you want the serialization logic to be as close as possible to the actual class. It just sat weird with me, much like the //components split does, because it requires knowing when it's "ok" to have something like that (e.g. it's OK for mojo) and when it's not (e.g. something like //net/foo depending on //content). Hence the email - just trying to understand if this was OK.

 

For old chrome IPC, there are also changes happening now to move traits to "ipc" subdirectories in ui/gfx/ipc, url/ipc, media/ipc, gpu/ipc. While chrome IPC is going to be replaced with mojo, the reason these directories are popping up is just to help the transition. In particular for the Mus work, there are command buffer related IPCs in content that need to be shared with Mus. More background in https://bugs.chromium.org/p/chromium/issues/detail?id=586364.

Also super-helpful, because I was equally surprised by //url/ipc :)

 

Can you point us at the cl?

https://codereview.chromium.org/1853643007

 

Is this related to Mus?

No, this was related to remoting.

 

ipc/ doesn't depend on net/, so I'm curious what circular dependency you're thinking of?

Well, as you pointed out with your ASCII art, for the case of ipc/, I thought of them as sibling directories. Having //net (or a subdirectory) depend on //ipc (for the param traits) seems like it means that //ipc sits below //net, and that seemed like a large enough dependency change that I wanted to push back on.

Mojo was circular, IPC was sibling.

 

Will Chan had, in the past, told and taught me that directory-level circular dependencies are, in many ways, similar to class-level circular dependencies, and that both should be avoided, and so that's the background I'm coming from. Just trying to make sense of this, and figure out what the right way to think of these layers as - and whether I was wrong in arguing that //net shouldn't know about //ipc.

These are all valid guidelines and we should continue to stick to them.

Even the last remark? It sounds like, based on your reply, that having a //net/ipc would be acceptable and be the exception to the above - as would having a //net/mojo.

[John Abd-El-Malek]

On Wed, Apr 6, 2016 at 10:11 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Wed, Apr 6, 2016 at 9:49 PM, John Abd-El-Malek <j...@chromium.org> wrote:

With old chrome IPC, the layering was much simpler. i.e. roughly

[chrome] [components]

               [content]

[net]           [ipc]          [blink]  

        [base]

Right, this was my understanding.

 

To solve this problem, for "low-level" classes like gfx::Rect or URL, we're putting the mojo version of traits in those subdirectories. That way components that sit below content can still send GURL or gfx::Rect in mojoms. These traits are in a mojo subdirectory, and are in separate build targets so that depending on src/ui or src/url doesn't bring in a dependency on mojo.

OK. I mean, it totally makes sense on one hand, because you want the serialization logic to be as close as possible to the actual class. It just sat weird with me, much like the //components split does, because it requires knowing when it's "ok" to have something like that (e.g. it's OK for mojo) and when it's not (e.g. something like //net/foo depending on //content). Hence the email - just trying to understand if this was OK.

One way to think about it is that mojo (at least the parts needed to define interfaces and traits) are now considered low level. Chrome IPC is turning into the same as well.

 

 

For old chrome IPC, there are also changes happening now to move traits to "ipc" subdirectories in ui/gfx/ipc, url/ipc, media/ipc, gpu/ipc. While chrome IPC is going to be replaced with mojo, the reason these directories are popping up is just to help the transition. In particular for the Mus work, there are command buffer related IPCs in content that need to be shared with Mus. More background in https://bugs.chromium.org/p/chromium/issues/detail?id=586364.

Also super-helpful, because I was equally surprised by //url/ipc :)

 

Can you point us at the cl?

https://codereview.chromium.org/1853643007

Yeah I think for this, (chrome IPC) traits for net::IPAddress should probably be in net/ipc to match what we're doing with other top level directories.

 

Is this related to Mus?

No, this was related to remoting.

 

ipc/ doesn't depend on net/, so I'm curious what circular dependency you're thinking of?

Well, as you pointed out with your ASCII art, for the case of ipc/, I thought of them as sibling directories. Having //net (or a subdirectory) depend on //ipc (for the param traits) seems like it means that //ipc sits below //net, and that seemed like a large enough dependency change that I wanted to push back on.

Mojo was circular, IPC was sibling.

Currently, IPC has also become very low level that many other dirs can depend on (i.e. recently src/gpu, src/media, src/ui, src/url).

 

 

Will Chan had, in the past, told and taught me that directory-level circular dependencies are, in many ways, similar to class-level circular dependencies, and that both should be avoided, and so that's the background I'm coming from. Just trying to make sense of this, and figure out what the right way to think of these layers as - and whether I was wrong in arguing that //net shouldn't know about //ipc.

These are all valid guidelines and we should continue to stick to them.

Even the last remark? It sounds like, based on your reply, that having a //net/ipc would be acceptable and be the exception to the above - as would having a //net/mojo.

Sorry I misread that last sentence as net can't depend on ipc because of the mojo network service. You're right, having net/ipc and net/mojo is now fine in this new world.

[Daniel Cheng]

On Wed, Apr 6, 2016 at 10:12 PM Ryan Sleevi <rsl...@chromium.org> wrote:

On Wed, Apr 6, 2016 at 9:49 PM, John Abd-El-Malek <j...@chromium.org> wrote:

With old chrome IPC, the layering was much simpler. i.e. roughly

[chrome] [components]

               [content]

[net]           [ipc]          [blink]  

        [base]

Right, this was my understanding.

 

To solve this problem, for "low-level" classes like gfx::Rect or URL, we're putting the mojo version of traits in those subdirectories. That way components that sit below content can still send GURL or gfx::Rect in mojoms. These traits are in a mojo subdirectory, and are in separate build targets so that depending on src/ui or src/url doesn't bring in a dependency on mojo.

OK. I mean, it totally makes sense on one hand, because you want the serialization logic to be as close as possible to the actual class. It just sat weird with me, much like the //components split does, because it requires knowing when it's "ok" to have something like that (e.g. it's OK for mojo) and when it's not (e.g. something like //net/foo depending on //content). Hence the email - just trying to understand if this was OK.

 

For old chrome IPC, there are also changes happening now to move traits to "ipc" subdirectories in ui/gfx/ipc, url/ipc, media/ipc, gpu/ipc. While chrome IPC is going to be replaced with mojo, the reason these directories are popping up is just to help the transition. In particular for the Mus work, there are command buffer related IPCs in content that need to be shared with Mus. More background in https://bugs.chromium.org/p/chromium/issues/detail?id=586364.

Also super-helpful, because I was equally surprised by //url/ipc :)

 

Can you point us at the cl?

https://codereview.chromium.org/1853643007

 

Is this related to Mus?

No, this was related to remoting.

 

ipc/ doesn't depend on net/, so I'm curious what circular dependency you're thinking of?

Well, as you pointed out with your ASCII art, for the case of ipc/, I thought of them as sibling directories. Having //net (or a subdirectory) depend on //ipc (for the param traits) seems like it means that //ipc sits below //net, and that seemed like a large enough dependency change that I wanted to push back on.

At least the way I think of it, //url/ipc and //url/mojo don't sit below //url so much as alongside it (even though they're subdirs): they just provide helpers for serializing/deserializing between processes. Thus, it actually provide isolation from //ipc and //mojo deps for the rest of the component, which may not care about ipc at all.

There are also other advantages to putting the serialization code for a type near the type's definition:

• Moving messages will become simpler since you don't need to find a new home for all the ParamTraits that the moved message depend on.
• More important, it reduces the chances of an accidental ODR violation: it seems like it'd be pretty to accidentally introduce one today since the ParamTraits are scattered throughout the entire codebase.

[Xiaohan Wang (王消寒)]

On Wed, Apr 6, 2016 at 11:01 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Wed, Apr 6, 2016 at 10:12 PM Ryan Sleevi <rsl...@chromium.org> wrote:

On Wed, Apr 6, 2016 at 9:49 PM, John Abd-El-Malek <j...@chromium.org> wrote:

With old chrome IPC, the layering was much simpler. i.e. roughly

[chrome] [components]

               [content]

[net]           [ipc]          [blink]  

        [base]

Right, this was my understanding.

 

To solve this problem, for "low-level" classes like gfx::Rect or URL, we're putting the mojo version of traits in those subdirectories. That way components that sit below content can still send GURL or gfx::Rect in mojoms. These traits are in a mojo subdirectory, and are in separate build targets so that depending on src/ui or src/url doesn't bring in a dependency on mojo.

OK. I mean, it totally makes sense on one hand, because you want the serialization logic to be as close as possible to the actual class. It just sat weird with me, much like the //components split does, because it requires knowing when it's "ok" to have something like that (e.g. it's OK for mojo) and when it's not (e.g. something like //net/foo depending on //content). Hence the email - just trying to understand if this was OK.

 

For old chrome IPC, there are also changes happening now to move traits to "ipc" subdirectories in ui/gfx/ipc, url/ipc, media/ipc, gpu/ipc. While chrome IPC is going to be replaced with mojo, the reason these directories are popping up is just to help the transition. In particular for the Mus work, there are command buffer related IPCs in content that need to be shared with Mus. More background in https://bugs.chromium.org/p/chromium/issues/detail?id=586364.

Also super-helpful, because I was equally surprised by //url/ipc :)

 

Can you point us at the cl?

https://codereview.chromium.org/1853643007

 

Is this related to Mus?

No, this was related to remoting.

 

ipc/ doesn't depend on net/, so I'm curious what circular dependency you're thinking of?

Well, as you pointed out with your ASCII art, for the case of ipc/, I thought of them as sibling directories. Having //net (or a subdirectory) depend on //ipc (for the param traits) seems like it means that //ipc sits below //net, and that seemed like a large enough dependency change that I wanted to push back on.

At least the way I think of it, //url/ipc and //url/mojo don't sit below //url so much as alongside it (even though they're subdirs): they just provide helpers for serializing/deserializing between processes. Thus, it actually provide isolation from //ipc and //mojo deps for the rest of the component, which may not care about ipc at all.

This is also how we structure //media.

The core "media" target knows nothing about mojo or IPC. Actually there are cases where people want to use "media" as a component in a single-processed environment and they don't care about mojo/IPC at all.

Then we have media/mojo which extends the core "media" to provide a way to use "media" in a multiple-processed environment (by providing the mojo interface, proxy, service etc). We have similar code structure in media/gpu/ipc.

I like our current model since it provides clear separation. The only annoying thing I found is that for a lot of C++ stuctures/enums I have to define a mojom one and implement the conversion from C++ to mojom and vice versa.  I though about using the mojom interface everywhere but that will "contaminate" our core "media" by pulling mojo dependencies.

There are also other advantages to putting the serialization code for a type near the type's definition:

• Moving messages will become simpler since you don't need to find a new home for all the ParamTraits that the moved message depend on.
• More important, it reduces the chances of an accidental ODR violation: it seems like it'd be pretty to accidentally introduce one today since the ParamTraits are scattered throughout the entire codebase.

 

Mojo was circular, IPC was sibling.

 

Will Chan had, in the past, told and taught me that directory-level circular dependencies are, in many ways, similar to class-level circular dependencies, and that both should be avoided, and so that's the background I'm coming from. Just trying to make sense of this, and figure out what the right way to think of these layers as - and whether I was wrong in arguing that //net shouldn't know about //ipc.

These are all valid guidelines and we should continue to stick to them.

Even the last remark? It sounds like, based on your reply, that having a //net/ipc would be acceptable and be the exception to the above - as would having a //net/mojo.

--

You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To post to this group, send email to chromi...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAF3XrKr7TowuVXFOVPOeY%2Bon7FiH7NH%2BECTuRAApsF0QH0X8gw%40mail.gmail.com.

[John Abd-El-Malek]

I think we want to avoid duplicate structures in general. There are two choices in this scenario:

-let single process code depend on structs declared in mojoms. IMO this is fine since the mojo generated structs/enums don't have dependencies on multi-process and don't require pipes.

-if this code is only called by C++ and will never be called from other languages, you can send structs with IPC's ParamTraits over mojo