IPDL: Allow Shmems to be shared across different protocol trees

RESOLVED FIXED

Status

()

Core
IPC
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: romaxa, Assigned: cjones)

Tracking

(Depends on: 1 bug)

Trunk
Other
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 442496 [details] [diff] [review]
Patch which is reproducing the problem

I'm trying to allocate memory in gfxSharedImageSurface with ContentProcessParent::GetSingleton()->AllocMem.. and it is not possible to send that memory to child process if we are using Shmem canvas stuff...

Apply attached patch on latest e10s.
run normal fennec with this branch:
./run-mozilla.sh ./xulrunner-bin /tmp/fennec_non_e10s/application.ini -graphicssystem raster -url http://www.communitymx.com/content/source/E5141/wmodeopaque.htm

Result is:
>>>>>>Func:NPP_SetWindow::540 shmem:0xaec2f000
>>>>>>Func:NPP_SetWindow::547 shmem2:0xaec07000
>>>>>>Func:NPP_HandleEvent::724 shmem:0xaec2f000
>>>>>>Func:AnswerNPP_HandleEvent_Shmem::667 mem:0xb1167000
>>>>>>Func:NPP_HandleEvent::728 shmem:0xaec2f000
>>>>>>Func:NPP_HandleEvent::730 shmem2:0xaec07000
>>>>>>Func:AnswerNPP_HandleEvent_Shmem::667 mem:0xb1167000
>>>>>>Func:NPP_HandleEvent::734 shmem2:0xaec2f000
>>>>>>Func:NPP_HandleEvent::724 shmem:0xaec2f000
>>>>>>Func:AnswerNPP_HandleEvent_Shmem::667 mem:0xb1167000
>>>>>>Func:NPP_HandleEvent::728 shmem:0xaec2f000
>>>>>>Func:NPP_HandleEvent::730 shmem2:0xaec2f000
.....

USE_SHM_CANVAS=1 ./run-mozilla.sh ./xulrunner-bin /tmp/fennec_non_e10s/application.ini -graphicssystem raster -url http://www.communitymx.com/content/source/E5141/wmodeopaque.htm

Result is:
>>>>>>Func:NPP_SetWindow::540 shmem:0xacc30000
>>>>>>Func:NPP_SetWindow::547 shmem2:0xacc08000
>>>>>>Func:NPP_HandleEvent::724 shmem:0xacc30000
>>>>>>Func:AnswerNPP_HandleEvent_Shmem::667 mem:0xb1067000
>>>>>>Func:NPP_HandleEvent::728 shmem:0xacc30000
>>>>>>Func:NPP_HandleEvent::730 shmem2:0xacc08000
>>>>>>Func:AnswerNPP_HandleEvent_Shmem::667 mem:(nil)
>>>>>>Func:NPP_HandleEvent::734 shmem2:(nil)
>>>>>>Func:NPP_HandleEvent::724 shmem:0xacc30000
>>>>>>Func:AnswerNPP_HandleEvent_Shmem::667 mem:0xb1067000
>>>>>>Func:NPP_HandleEvent::728 shmem:0xacc30000
>>>>>>Func:NPP_HandleEvent::730 shmem2:(nil)



Any ideas where is the problem?
(Reporter)

Comment 1

8 years ago
Created attachment 442499 [details] [diff] [review]
More simple changes ALLOC_WITH_SINGLETON=1

Looks like singleton allow to create only 2 Shmem instances which are working correctly with IPC
3-rd instance is created, but when I'm sending it to another process it getting 0 value.
run the same stuff without any env var:

>>>>>>Func:NPP_HandleEvent::758 shmem3:0xacdf8000
>>>>>>Func:AnswerNPP_HandleEvent_Shmem::667 mem:0xb1006000
>>>>>>Func:NPP_HandleEvent::762 shmem3:0xacdf8000



export ALLOC_WITH_SINGLETON=1, shmem3 is not initialized correctly on child process
>>>>>>Func:NPP_HandleEvent::758 shmem3:0xad020000
>>>>>>Func:AnswerNPP_HandleEvent_Shmem::667 mem:(nil)
>>>>>>Func:NPP_HandleEvent::762 shmem3:(nil)
Attachment #442496 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Summary: ShMemory allocated with ContentProcessParent::GetSingleton() does not work always → ContentProcessParent::GetSingleton() allocating only 2 sharable Shmem objects
I'm surprised this isn't abort()ing on you.

The changes needed to support this are

 - Refcount |SharedMemory|s.  Needed because they're no longer necessarily exclusively "owned" by a protocol tree.

 - Add a new IPDL interface |AdoptShmem(const Shmem& x)| to the existing AllocShmem()/DeallocShmem().  This should abort if x.mSegment is already in toplevelActor.mShmems.  If not, then x.mSegment should be AddRef()d, and the SHMEM_CREATED message sent to the other process the same as for AllocShmem().

With this, your code could basically do (if this is what you want)

  Shmem x = ContentProcessChild->AllocShmem();
  //...
  PluginModuleParent->AdoptShmem(x);
  instance->Draw(x);

Note that the same single-thread ownership semantics as before still apply in this scheme.  In the case above, this means that only one of PluginModuleChild, PluginModuleParent/ContentProcessChild, or ContentProcessParent can access the Shmem at any given time.
Summary: ContentProcessParent::GetSingleton() allocating only 2 sharable Shmem objects → IPDL: Allow Shmems to be shared across different protocol trees
Blocks: 562770
(Reporter)

Comment 3

8 years ago
I want to allocShmem in gfxSharedImageSurface class which can be created in any process... (plugin/content/chrome) and shared with any process plugin->chrome, content->plugin e.t.c...


Practically I want to do something like:
Shmem x = ContentProcessCurrent->AllocShmem();
 and make sure that I can send this memory to child or parent processes...
(Reporter)

Updated

8 years ago
Depends on: 562285
(In reply to comment #3)
> I want to allocShmem in gfxSharedImageSurface class which can be created in any
> process... (plugin/content/chrome) and shared with any process plugin->chrome,
> content->plugin e.t.c...

Hard-coding gfxSharedImageSurface to use particular actors for Shmem allocation doesn't seem like a good idea to me.  You could make whatever your gfxSharedImageSurface::Create()-like method is be templated on a "Shmem allocator" parameter.

You can share the Shmem to any process using a combination of AdoptShmem() and IPDL messages.
This doesn't depend on the new gfxSharedImageSurface, although it might block that.
No longer depends on: 562285
Created attachment 442616 [details] [diff] [review]
Refcount |ShareMemory|s
Assignee: nobody → jones.chris.g
Attachment #442616 - Flags: review?(joe)
Created attachment 442617 [details] [diff] [review]
Allow |Shmem|s to be shared across different protocol trees
Attachment #442617 - Flags: review?(bent.mozilla)
Oleg, I only tested these patches pass the existing unit tests; it would be very hard to add a new test for these with the current framework (that will be changing soon, though!).  (Also, fennectrolysis crashes almost immediately on startup for me.)  I'd appreciate it if you could test these out.
Created attachment 442620 [details] [diff] [review]
Allow |Shmem|s to be shared across different protocol trees

Oops, forgot a critical AddRef().
Attachment #442617 - Attachment is obsolete: true
Attachment #442620 - Flags: review?(bent.mozilla)
Attachment #442617 - Flags: review?(bent.mozilla)
(Reporter)

Comment 10

8 years ago
Comment on attachment 442620 [details] [diff] [review]
Allow |Shmem|s to be shared across different protocol trees


>+        adoptShmem.addstmt(StmtDecl(Decl(_rawShmemType(ptr=1), rawvar.name),
>+                                    init=_shmemSegment(memvar)))
>+        ifbad = StmtIf(ExprBinary(
>+            ExprNot(rawvar), '||',
>+            ExprCall(ExprVar('IsTrackingSharedMemory'), args=[ rawvar ])))

I've tested this patch and found one problem...
After AdoptShmem I must keep adopted memory and remember it somewhere... because second adoptShmem will abort me.
Can we remove check for IsTrackingSharedMemory here?
Comment on attachment 442616 [details] [diff] [review]
Refcount |ShareMemory|s

General comment:

8 lines of context is easier to review, though a *little* more problematic when it comes to merging in new revisions (more fuzz to go wrong). Still, this is what I have in my ~/.hgrc:

[diff]
git = 1
showfunc = 1
unified = 8

[qdiff]
git = 1
showfunc = 1
unified = 8

However!! this still looks fine. :)
Attachment #442616 - Flags: review?(joe) → review+
(In reply to comment #10)
> (From update of attachment 442620 [details] [diff] [review])
> 
> >+        adoptShmem.addstmt(StmtDecl(Decl(_rawShmemType(ptr=1), rawvar.name),
> >+                                    init=_shmemSegment(memvar)))
> >+        ifbad = StmtIf(ExprBinary(
> >+            ExprNot(rawvar), '||',
> >+            ExprCall(ExprVar('IsTrackingSharedMemory'), args=[ rawvar ])))
> 
> I've tested this patch and found one problem...
> After AdoptShmem I must keep adopted memory and remember it somewhere...
> because second adoptShmem will abort me.
> Can we remove check for IsTrackingSharedMemory here?

That's a sanity check I added because couldn't think of any cases where code might be unsure whether it had already adopted a Shmem into another protocol tree.  Per our IRC chat, it seems that you're doing this to avoid storing away a Shmem for the PluginModule tree.  That's not going to work, I don't think.  There are two problems.

The first problem is that different protocol trees will use different identifiers (~=Shmem) to refer to the same underlying OS shmem region.  The reason for this is that it's hard to get an identifier for Shmems that's globally unique across all processes.  We could do this on linux, by grubbing around in linux-specific interfaces.  I don't know if we could on windows, and I doubt it.  Dunno about mac.  Creating globally unique identifiers in Gecko is possible, but it'd be tricky and expensive.  The current strategy works and is simple and cross-platform, so let's stick with that until we have a compelling reason not to.

The second problem is that Shmems aren't "smart pointers" to shmem regions, but rather handles (like windows's HANDLE).  They need to be explicitly deallocated through DeallocShmem() (~=CloseHandle()).  The addition of AdoptShmem() and having a Shmem-identifier-per-protocol-tree also means that Shmems have to be DeallocShmem()d for each protocol tree they've been adopted into, otherwise they'll "leak" for the lifetime of the longest-lived protocol tree.  In your case, you need to DeallocShmem() for both PContent and PPluginModule.  We could change this though, but we'd need a stronger notion of Shmem ownership ("owning actor") than the current one, and that would limit Shmem flexibility somewhat (Shmems couldn't outlive their owning actors, which means they could only be passed in messages for that actor's managee subtree).  I honestly don't know which approach is best; I'd like to let the cross-process layers work inform it.
(Reporter)

Comment 13

8 years ago
I've updated my build and it works fine now for me.
Comment on attachment 442620 [details] [diff] [review]
Allow |Shmem|s to be shared across different protocol trees

Looks good, only question I have is about the HasData changes... How many of these shared things do we expect to have? Will that get slow?

r=me for now, maybe file a followup on fixing that?
Attachment #442620 - Flags: review?(bent.mozilla) → review+
(In reply to comment #14)
> (From update of attachment 442620 [details] [diff] [review])
> Looks good, only question I have is about the HasData changes... How many of
> these shared things do we expect to have? Will that get slow?
> 

Approximately 2 per "leaf" gfx layer, which atm are ~video, canvas, (soon to be) plugins, and then "everything else".  espn.com has 5-8, if I read my debug spew correctly.  So a realistic bad-ish case might be O(1000), although in theory the number is bounded only by available DRAM.

> r=me for now, maybe file a followup on fixing that?

Sounds good to me.
Depends on: 567582
http://hg.mozilla.org/mozilla-central/rev/4254363b9c63
http://hg.mozilla.org/mozilla-central/rev/2390945942d6

Pushed without specific tests (sorry! it's hard atm), but with the understanding that Oleg has been using these patches.  Filed followup bug 567582 for O(n) AdoptShmem().
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.