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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s ? ---
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: froydnj)

References

Details

(Whiteboard: [MemShrink] btpp-active)

Attachments

(2 files)

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.
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
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)
Assignment works just fine here, is clearer, and avoids unnecessary work.
Attachment #8743254 - Flags: review?(continuation)
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+
Attachment #8743254 - Flags: review?(continuation) → review+
Whiteboard: [MemShrink] → [MemShrink] btpp-active
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?
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+
You need to log in before you can comment on or make changes to this bug.