1255435 - Helper classes to manage actors without depending on IPDL

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

This is going to be a bit controversial since it duplicates one of ipdl's functionalities, but I ran into bug 1254591 while trying to make progress in bug 1128503. I decided to reimplement the glue that maintains actors on each side (mostly two glorified std::maps...) outside of ipdl, and built a destruction sequence similar to PTexture and PCompositable into it. It was way easier than figuring out how to fix the IPDL compiler, and as a bonus it is simpler to unit test things that are built around actors (don't need the whole IPDL machinery, just to mock a simple pipe-like interface to send the messages between the actors (see the gtest)).

This will be used by the cross process allocator (each allocator has an actor pair that we need to refer to in a SurfacDescriptor, which I haven't managed to do with ipdl actors because of bug 1254591).

I don't think this should become the default way to manage actors since IPDL conveniently generates some of the code that you need to add by hand with this system (actor constructors and demultiplexing the incoming messages), but it should come handy when running into the kind of issues I am hitting or when we simply want to not depend on ipdl.

Ideally we could integrate this into IPDL rather than have this functionality generated by the ipdl compiler (fixing bugs in the generator is awful) and only generate the remaining glue, but I am lacking the motivation to do it at the moment.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Implement "external" Actor helpers.

Implement the helper classes wire them up in PImageBridge and add a gtest.

Comment 2

[Nicolas Silva [:nical]]

Comment on attachment 8729050 [details] [diff] [review]
Implement "external" Actor helpers.

Review of attachment 8729050 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, I am exploring a different way to tackle the cross-process allocator business that may not need this (not sure yet).

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

If there's some special thing that gfx code does that we don't expect other code to do, then this seems fine. But the motivation here seems pretty lame. I'll look into bug 1254591 tomorrow. In general, we should fix bugs in IPDL so that everyone benefits from them.

I totally agree that the way actor destruction works in IPDL is terrible. But this is something that trips up everyone who uses IPDL. I'd like to make a handshake sequence like this built into IPDL so that no one has to do anything special. I've been meaning to do this for a while. Now that all the CPOW bugs are pretty much fixed, I hope to have time for it soon. It sounds like this isn't the primary motivation for this bug anyway since the gfx code already implements something like this.

Anyway, I don't want to close the door on something like this, but I think it needs more justification.

Comment 4

[Nicolas Silva [:nical]]

(In reply to Bill McCloskey (:billm) from comment #3)

I agree with all of what you are saying. Currently I find that fixing bugs in ipdl's generator is a lot harder than modifying actual c++ code. If other people don't get blocked by the same issues as me, it's not always clear that doing the right thing is worth the extra time it takes. We should schedule some time in London to talk about ipdl features and the things that are (or I find to be) difficult to fiddle with.

> I'd like to make a handshake sequence like this built into IPDL

This would be great.

> Anyway, I don't want to close the door on something like this, but I think
> it needs more justification.

Let's see how bug 1254591 goes and/or if I can manage to make the allocator work without needing actors (or other mechanism to pair things across processes) which is another workaround but at least doesn't involve duplicating something that already exists.

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 8729050 [details] [diff] [review]
Implement "external" Actor helpers.

Lets shelve this for now.