Closed Bug 1272018 Opened 8 years ago Closed 8 years ago

Use shared memory to send images for drag and drop

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: mccr8, Assigned: cyu)

References

Details

Attachments

(2 files)

Currently, drag and drop sends images by serializing the image data to a string, then copying that string into an IPC message (see the various callers of nsContentUtils::GetSurfaceData). This can result in large IPC messages, as seen in bug 1271102. Shared memory would probably be a better way to do this, and I think we already have some kind of setup for sharing image data via shared memory so maybe it wouldn't be too hard. Generalized drag and drop involves some entire data structure, so maybe it would be.
tracking-e10s: --- → ?
Blocks: e10s-oom
Priority: -- → P1
Kan-ru, could somebody on your team look into this? It sounds like it could be straightforward for somebody who understands how shared memory works for image data.
Flags: needinfo?(kchen)
I can take a look.
Assignee: nobody → cyu
Nical, I know we have the mechanism to share surfaces across processes. The content process creates a DataSourceSurface for transmission by copying and the receiver in parent process also creates a DataSourceSurface out of the raw data by copying. I think we can eliminate one of the two copies here by using some kind of shared surface. Do you know if there is something we can reuse?
Flags: needinfo?(kchen) → needinfo?(nical.bugzilla)
Attached patch WIPSplinter Review
This uses Shmem for passing the image data.
TODO: remove the extra copy in nsDragServiceProxy::InvokeDragSessionImpl().
Even further, find a way to share the surface to even reduce the number of copies so that the child process allocates and fills the shared memory, and the parent process uses the shared memory without making another copy and deallocates it after done with it.
See Also: → 1275398
Wait, is this bug about using shared memory for the drag feedback image, or for the actual images being dragged? Or both? The patch suggests the drag feedback image only.
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #3)
> Nical, I know we have the mechanism to share surfaces across processes. The
> content process creates a DataSourceSurface for transmission by copying and
> the receiver in parent process also creates a DataSourceSurface out of the
> raw data by copying. I think we can eliminate one of the two copies here by
> using some kind of shared surface. Do you know if there is something we can
> reuse?

The standard texture sharing mechanism is to create a TextureClient/TextureHost pair. It gives you a way to access it as a DrawTarget on the child side so that you can render what you need in there, and as a TextureSource on the parent side, which is what the Compositor knows how to draw, all of that without extra copy (Assuming we use a Compositor to draw the image when drag-n-drop'ing, I'm not sure, if it's not the case we could make TextureHost expose a SourceSurface too).
PContent doesn't manage PTexture so if you want to use it here there's a little it of ipdl plumbing to add but nothing too complicated.
The other advantage of use TextureClient/Host is that you are not limited to sharing shmems, you can also use, say, DXGI surfaces if it's more efficient for your use case without adding code.

That said, the existing code is already based on using data surfaces and share some buffers, so just replacing the nsCString by a Shmem like in Cervantes's patch is already a notable win with minimal code changes.
Flags: needinfo?(nical.bugzilla)
Attachment #8756297 - Flags: review?(enndeakin)
(In reply to Neil Deakin from comment #6)
> Wait, is this bug about using shared memory for the drag feedback image, or
> for the actual images being dragged? Or both? The patch suggests the drag
> feedback image only.

I filed bug 1275398 about the string usage in IPCDataTransferData, which should cover the images being dragged (plus clipboard stuff). I didn't want to pile too much stuff into this bug. They should be fairly orthogonal, though similar code.
Comment on attachment 8756297 [details]
MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8

https://reviewboard.mozilla.org/r/55074/#review52164

::: gfx/2d/DataSurfaceHelpers.h:12
(Diff revision 1)
>  #define _MOZILLA_GFX_DATASURFACEHELPERS_H
>  
>  #include "2D.h"
>  
>  #include "mozilla/UniquePtr.h"
> +#include "nsDebug.h"

Code in Moz2D can only depend on MFBT classes.
Attachment #8756297 - Flags: review?(bas)
Comment on attachment 8756297 [details]
MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8

This seems to work ok, but I think Andrew would be a better reviewer of this.
Attachment #8756297 - Flags: review?(enndeakin)
Comment on attachment 8756297 [details]
MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55074/diff/1-2/
Attachment #8756297 - Attachment description: MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?enndeakin,bas → MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8
Attachment #8756297 - Flags: review?(continuation)
Comment on attachment 8756297 [details]
MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8

This should get reviewed by somebody who is familiar with shared memory IPC. The patch looks straightforward otherwise.
Attachment #8756297 - Flags: review?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #13)
> This should get reviewed by somebody who is familiar with shared memory IPC.
> The patch looks straightforward otherwise.

I looked at the patch and the shmem stuff looks good.
We have a layers::ShmemAllocator interface identical to the IShmemAllocator added here, so we should remove the one in layers as a followup if this lands.
Attachment #8756297 - Flags: review?(nical.bugzilla)
Comment on attachment 8756297 [details]
MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8

https://reviewboard.mozilla.org/r/55074/#review53928
Attachment #8756297 - Flags: review?(nical.bugzilla) → review+
See Also: → 1274075
Blocks: 1171307
(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #16)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b3715489009
> Perm fails in GPU tests to be fixed.

You probably just need to update to a newer parent rev to make those gpu failures go away.
https://hg.mozilla.org/mozilla-central/rev/d3571746fb0b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Cervantes, should we uplift this e10s bug fix from Nightly 50 to Aurora 49 or even Beta 48? We plan to enable e10s by default for some release channel users with Firefox 48.
Flags: needinfo?(cyu)
Since I think we should uplift this to 49 since for limitation of IPC message size. I would leave 48 as is since this isn't a must-have fix.
Flags: needinfo?(cyu)
Comment on attachment 8756297 [details]
MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8

Approval Request Comment
[Feature/regressing bug #]: No bug. e10s internals for transferring drag/drop image data.
[User impact if declined]: Tab crashes if the user drags very large image. This is very unusual, but when it happens the user has no workaround. Even restarting the browser doesn't help.
[Describe test coverage new/current, TreeHerder]: Landed/autotested on m-c.
[Risks and why]: Low to medium. We are not sure whether shared memory has higher chance failing in allocation. If it fails to allocate shared memory, the user will fail to drag an image. Restarting the browser may help if the allocation of large shared memory succeeds after restart.
[String/UUID change made/needed]: None.
Attachment #8756297 - Flags: approval-mozilla-aurora?
Thanks. wontfix for Beta 48 based on Cervantes' comment 21.
Comment on attachment 8756297 [details]
MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8

Fix for crashes from large image copying, let's try it in aurora. 
cyu, do you have any suggested ways to test/verify this fix?
Flags: needinfo?(cyu)
Attachment #8756297 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24)
> Comment on attachment 8756297 [details]
> MozReview Request: Bug 1272018 - Use shared memory to transfer drag image
> data. r?mccr8
> 
> Fix for crashes from large image copying, let's try it in aurora. 
> cyu, do you have any suggested ways to test/verify this fix?

Dragging a 70 MP image and see if the tab crashes. Please make sure the system has enough free memory so that it doesn't hit OOM.
Flags: needinfo?(cyu)
Blocks: 1295272
See Also: → 1336417
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: