Closed
Bug 1265902
Opened 8 years ago
Closed 8 years ago
Avoid copying the return value of nsContentUtils::GetSurfaceData()
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: mccr8, Assigned: froydnj)
References
Details
(Whiteboard: [MemShrink] btpp-active)
Attachments
(2 files)
2.81 KB,
patch
|
mccr8
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.63 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
Currently, nsContentUtils::GetSurfaceData() creates a new mozilla::UniquePtr<char[]> and returns it. It is up to the three callers to deal with this properly. 1) nsDragServiceProxy::InvokeDragSessionImpl() stores it into an nsDependentCString, which avoids the copy 2) PuppetWidget::SetCursor() stores it into an nsCString, which does a copy. This could be easily fixed by changing that to nsDependentCString. 3) nsContentUtils::TransferableToIPCTransferable stores it into an nsCString. Maybe some kind of string adopt could be used. It might be better to just change this method so it returns a string, and you don't have to worry about callers messing something up and creating a copy. Anyways, these extra copies are bad, because maybe at least (3) could cause a copy of a large image under some circumstances. I've also started trying to figure out how to avoid the intermediate string entirely, by doing an IPC write from the image data directly into an IPC::Message, but that is more complicated.
Assignee | ||
Comment 1•8 years ago
|
||
Fixing the string copies is pretty straightforward. The better way to fix this would be to remove nsContentUtils::GetSurfaceData and introduce an AutoSurfaceData that exposes the mapped buffer pointer and the buffer length, and then the client can copy the buffer only if they need to. I can fix the string copies for now.
Assignee: nobody → nfroyd
Assignee | ||
Comment 2•8 years ago
|
||
nsContenUtils::GetSurfaceData() returns an allocated buffer; the uses of it in PuppetWidget and nsContentUtils::TransferableToIPCTransferable copy the allocated buffer again. We can do better than this. In the PuppetWidgetCase, we can simply construct an nsDependentCString for sending the IPC message. In the TransferableToIPCTransferable case, we can transfer ownership of the buffer into the IPCDataTransferItem we are creating.
Attachment #8743253 -
Flags: review?(continuation)
Assignee | ||
Comment 3•8 years ago
|
||
Assignment works just fine here, is clearer, and avoids unnecessary work.
Attachment #8743254 -
Flags: review?(continuation)
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Comment 4•8 years ago
|
||
Comment on attachment 8743253 [details] [diff] [review] part 1 - be more efficient when using nsContentUtils::GetSurfaceData() Review of attachment 8743253 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this, Nathan! Anything involving strings takes me longer than it should. nit: "nsContenUtils" is missing a "t". ::: dom/base/nsContentUtils.cpp @@ +7405,5 @@ > nsContentUtils::GetSurfaceData(dataSurface, &length, &stride); > > IPCDataTransferItem* item = aIPCDataTransfer->items().AppendElement(); > item->flavor() = nsCString(flavorStr); > + // Turn item->data() into an nsCString prior to accessing it. Tricky...
Attachment #8743253 -
Flags: review?(continuation) → review+
Reporter | ||
Updated•8 years ago
|
Attachment #8743254 -
Flags: review?(continuation) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c25fa27b43 https://hg.mozilla.org/integration/mozilla-inbound/rev/01c33b05600e
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink] btpp-active
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c7c25fa27b43 https://hg.mozilla.org/mozilla-central/rev/01c33b05600e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8743253 [details] [diff] [review] part 1 - be more efficient when using nsContentUtils::GetSurfaceData() Approval Request Comment [Feature/regressing bug #]: e10s [User impact if declined]: increased memory usage when dragging and dropping images [Describe test coverage new/current, TreeHerder]: this code has some test coverage [Risks and why]: low. it just eliminates a copy. [String/UUID change made/needed]: none
Attachment #8743253 -
Flags: approval-mozilla-aurora?
Reporter | ||
Comment 8•8 years ago
|
||
I guess it isn't really worth backporting the second part. It doesn't seem like that would have an impact on memory usage.
Comment on attachment 8743253 [details] [diff] [review] part 1 - be more efficient when using nsContentUtils::GetSurfaceData() This fix should help improve e10s OOM situation, Aurora47+
Attachment #8743253 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 10•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/6bf374e42b5e
You need to log in
before you can comment on or make changes to this bug.
Description
•