Avoid duplicate copy of encoded images in Blink during load |
|||||||
Issue descriptionAt the moment, we appear to have three copies of the encoded form of every image during load. After an image has finished loading, one of the duplicates is removed. (*) This can result in a spike of memory during load, which is undesireable. One copy is passed along in the mojo IPC from the browser copy. A second copy is in a SharedBuffer owned by the ImageResource. A third is in a SkRWBuffer owned by the DeferredImageDecoder. It seems we can avoid creating the SharedBuffer in the first place, and when ImageResource::AppendData() is called, update the SkRWBuffer directly. To do so will be some refactoring work, however, perhaps with complications for multipart images. (*) https://codereview.chromium.org/2054643003/ implemented the behavior of deleting the second copy after load. https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7aciY/edit# describes the flow of information and updates.
,
Jul 13 2017
@hajimehoshi: what do you think of this idea? Worth doing? Harder than I think?
,
Jul 13 2017
,
Jul 13 2017
Chris, see issue 728627 which I think is exactly what you are also thinking of.
,
Jul 13 2017
Thanks for the pointer. 728627 sounds like a tracking bug to simplify things to the point that ideas like 741946 are possible. And we both agree that more investment in reducing the complexity here is desireable.
,
Jul 13 2017
The last point on 728627 is making SharedBuffer backed by a SkRWBuffer, which would remove the need for that third copy in your list. But splitting into a separate/more focused bug sgtm.
,
Jul 13 2017
Oh if that's what point (3) in the other bug means, then yes. :) Marking blocking bugs.
,
Jul 13 2017
Is this also related to Issue 727995 (draft CL: https://codereview.chromium.org/2918443003/)? Random thoughts about implementation: How does this interact with the UpdateImage() throttling introduced by https://codereview.chromium.org/2361263003? Tthe rhrottling requires buffering of data in ImageResource before passing the data to ImageResourceContent and blink::Image, and thus SharedBuffer might be still needed if we want to preserve this behavior, or we have to move throttling logic to blink::Image (add data always to SkRWBuffer, but throttle decoding). > perhaps with complications for multipart images. Agree... (we might want to keep SharedBuffer for multipart images, because probably we have to keep two buffers anyway, one (SkRWBuffer) for the previous part and one for the current part (SharedBuffer), but this might complicates the code)
,
Jul 13 2017
It's related, yes. One thing I didn't mention is that I surveyed the callsites to Data() for ImageResource, and there don't seem to be any callsites. All of them are for resources that are not images it seems. I also discussed the throttling w/vmpstr, and we agreed that that would need to be moved. The throttling can happen in ImageResourceContent, because the purpose of the throttling is to prevent over-invalidation of images during load, not to prevent the buffers from being filled. Finally, I agree that we can skip this optimization for multipart. These are very uncommon.
,
Jul 14 2017
> @hajimehoshi: what do you think of this idea? Worth doing? Harder than I think? The situation has been much changed as hiroshige@'s refactoring. Now it looks like ImageResource(Content) doesn't own SharedBuffer, right? Am I missing something? Anyway, I'm really happy with SharedBuffer's refactoring by fmalita@.
,
Jul 16
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 17
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by chrishtr@chromium.org
, Jul 13 2017