Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
559103002:
Reduce memory peaks when generating data urls for images. (Closed)
|
Created:
6 years, 3 months ago by Daniel Bratell Modified:
6 years, 3 months ago CC:
blink-reviews, jamesr, caseq+blink_chromium.org, krit, loislo+blink_chromium.org, Rik, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, jbroman, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, danakj, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionReduce memory peaks when generating data urls for images.
Adding Vector<char> to a String results in one (or more) temporary
copies of that Vector<char> since Vector isn't a pointer/smart pointer
like the types usually used.
Luckily there are many other ways to solve the problem. With StringBuilder
or using a String.
BUG=389085
(related to, not necessarily fixing it)
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181925
Patch Set 1 #
Total comments: 7
Patch Set 2 : toDataURL: Now with less parallel data. #Patch Set 3 : toDataURL - now without freeing Vector early. #
Total comments: 1
Patch Set 4 : toDataURL(): Fewer temporary coppies of image data #
Messages
Total messages: 24 (4 generated)
bratell@opera.com changed reviewers: + caseq@chromium.org, fs@opera.com, noel@google.com, pfeldman@chromium.org
noel, can you please take a look at the Platform/graphics/ImageBuffer.cpp changes. pfeldman/caseq can you please take a look at the change of InspectorLayerTreeAgent.cpp
inspector lgtm
noel@chromium.org changed reviewers: + noel@chromium.org
https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorLayerTreeAgent.cpp:389: url.reserveCapacity(url.length() + base64Data->size()); // Prevent over-allocation. nit: remove the comment. https://codereview.chromium.org/559103002/diff/1/Source/platform/graphics/Ima... File Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/559103002/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageBuffer.cpp:429: return EncodedImageToDataURL(encodedImage, mimeType); /curious would writing this as return "data:" + mimeType + ";base64," + base64Encode(encodedImage); do what you want here (interpolate a Sting)?
https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorLayerTreeAgent.cpp:388: url.append("data:image/png;base64,"); per https://trac.webkit.org/wiki/EfficientStrings, I think want .appendLiteral here.
pdr@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorLayerTreeAgent.cpp:387: StringBuilder url; Any reason for not using reserveCapacity before appending the string literal? You may have seen this already but the new string doc is a good read: https://docs.google.com/a/google.com/document/d/1kOCUlJdh2WJMJGDf-WoEQhmnjKLa...
Please take a new look. I also did an extra improvement in EncodedImageToDataURL to clear a buffer no longer needed before generating the final string. That should cut down another imagesize chunk of peak memory usage. https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorLayerTreeAgent.cpp:387: StringBuilder url; On 2014/09/11 04:38:15, pdr wrote: > Any reason for not using reserveCapacity before appending the string literal? > > You may have seen this already but the new string doc is a good read: > https://docs.google.com/a/google.com/document/d/1kOCUlJdh2WJMJGDf-WoEQhmnjKLa... I don't think I had seen that document before. It is a high level document though and I'm digging around in the dark sides of those objects. For example, it says that operator+ is good, but it's also O(n^2) in the number of pluses so it too can become a bad choice. The reason I didn't do reserveCapacity(22 + base64Data->size()) or reserveCapacity(sizeof("data:image/png;base64,") - 1 + base64Data->size()) or reserveCapacity(strlen("data:image/png;base64,") + base64Data->size()) was to make the code short and easy to read. This whole block will be totally dominated by the append of the image data so there is nothing to gain from optimizing it further. https://codereview.chromium.org/559103002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorLayerTreeAgent.cpp:389: url.reserveCapacity(url.length() + base64Data->size()); // Prevent over-allocation. On 2014/09/11 01:03:23, Noel Gordon wrote: > nit: remove the comment. Done. https://codereview.chromium.org/559103002/diff/1/Source/platform/graphics/Ima... File Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/559103002/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageBuffer.cpp:429: return EncodedImageToDataURL(encodedImage, mimeType); On 2014/09/11 01:03:23, Noel Gordon wrote: > /curious would writing this as > > return "data:" + mimeType + ";base64," + base64Encode(encodedImage); > > do what you want here (interpolate a Sting)? It would work the same. I put it in a separate method because it's done in two different methods, and despite being a 1-2 liner it is actually a fair bit of generated code (string constructions, destructions, copies, lots of StringAppend<> objects and it is unnecessary to have that more than once.
On 2014/09/11 11:22:32, Daniel Bratell wrote: > Please take a new look. > > I also did an extra improvement in EncodedImageToDataURL to clear a buffer no > longer needed before generating the final string. That should cut down another > imagesize chunk of peak memory usage. More like "might or might not". This is weird side-effect, and has not been shown the solve the problem you are attempting to solve. > to make the code short and easy to read. > https://codereview.chromium.org/559103002/diff/1/Source/platform/graphics/Ima... > File Source/platform/graphics/ImageBuffer.cpp (right): > > https://codereview.chromium.org/559103002/diff/1/Source/platform/graphics/Ima... > Source/platform/graphics/ImageBuffer.cpp:429: return > EncodedImageToDataURL(encodedImage, mimeType); > On 2014/09/11 01:03:23, Noel Gordon wrote: > > /curious would writing this as > > > > return "data:" + mimeType + ";base64," + base64Encode(encodedImage); > > > > do what you want here (interpolate a Sting)? > > It would work the same. Then please use it.
Also, I would check that using it does not add a terminating null to the dataURL reulst. Maybe measure the dataURL lenth returned by these routines before / after your change.
> On 2014/09/11 11:22:32, Daniel Bratell wrote: > > Please take a new look. > > > > I also did an extra improvement in EncodedImageToDataURL to clear a buffer no > > longer needed before generating the final string. That should cut down another > > imagesize chunk of peak memory usage. > > More like "might or might not". This is weird side-effect, and has not been > shown the solve the problem you are attempting to solve. What is a weird side effect? These data chunks can be many MB. The test I'm using is a 2048x2048 canvas that creates a 20 MB data url. The original code stored this first in a 18 MB Vector of encoded image, then copied to another 15 MB Vector, then to a temporary 20 MB Vector inside the concatenation and finally into a 20 MB String, all of these blocks alive at the same time so a total of 75 MB of peak memory usage. With the first patch the temporary Vector is removed for a total of 55 MB peak memory usage (in my measurements it looks like it drops more than 20 MB but I think it's noise). The last patch tried to make sure that the 15 MB Vector of encoded image didn't live at the same time as the last 20 MB of String, to reduce heap pressure by another 15 MB. Now String and Vector use different heaps so String can't immediately reuse the freed memory which means that the RAM will only be reused if the computer memory is under high pressure but I consider that a technical detail here. > Also, I would check that using it does not add a terminating null to the dataURL > reulst. Maybe measure the dataURL lenth returned by these routines before / > after your change. Same result (last byte is a '='). Nobody has added any extra characters at the end.
noel, please take a new look at ImageBuffer.cpp. It's now a minimal change.
The purpose here is to reduce peak memory usage and it seems there's been a 25% reduction for a dataURL with this change. Seems fine, but whether this change helps resolve the reported bug or not is unknown. There's no getting away from the fact that dataURL construction requires the image, the encodedImage, and the dataURL string to be live in memory at the same time. There are ways I can think of that could attain optimal peak usage, but they'd require more extensive changes. If the machine that's failing in the bug is sensitive to 20Meg, then perhaps that machine is on the verge of crashing. blink webkit code does not attempt to stave off an enivitable crash as a rule, preferring a clean null crash instead (less code, better security, and so on).
lgtm - with nits Also seek approval from pdr@ also before submitting. https://codereview.chromium.org/559103002/diff/40001/Source/platform/graphics... File Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/559103002/diff/40001/Source/platform/graphics... Source/platform/graphics/ImageBuffer.cpp:423: return "data:" + mimeType + ";base64," + base64Encode(encodedImage); nit - space before this line.
On 2014/09/12 15:20:20, Noel Gordon wrote: > The purpose here is to reduce peak memory usage and it seems there's been a 25% > reduction for a dataURL with this change. > Seems fine, but whether this change helps resolve the reported bug or not is > unknown. > > There's no getting away from the fact that dataURL construction requires the > image, the encodedImage, and the dataURL string to be live in memory at the same > time. There are ways I can think of that could attain optimal peak usage, but > they'd require more extensive changes. If the machine that's failing in the bug > is sensitive to 20Meg, then perhaps that machine is on the verge of crashing. > blink webkit code does not attempt to stave off an enivitable crash as a rule, > preferring a clean null crash instead (less code, better security, and so on). There will be (at least) one copy too much at peak still. In total we have 1. raw image (can't be let go). 2. jpeg/png encoded image (needed for step 3) 3. base64 encoded jpeg/png encoded image (needed for step 4) 4. data url string. Optimally it would be something like (1) -> (1, 2) -> (1, 2, 3) -> (1, 3) -> (1, 3, 4) -> (1, 4). After the last patch it will be (1) -> (1, 2) -> (1, 2, 3) -> (1, 2, 3, 4) -> (1, 2, 4) -> (1, 4) As for memory, I think in many cases the problem can be consecutive address space. If you have allocated yourself a 1 GB heap and used it a while, it might be hard to find 3-5 empty free blocks in the address space. It's also possible that the bug is something else, or that it's not 20 MB blocks but 200 MB blocks. I just noticed that you are not listed as OWNER for this code so I might have disturbed the wrong person. Sorry about that. Do you know who would be suitable to give the final green comment for platform/graphics/ImageBuffer.cpp?
Ah, pdr it is. I'll upload a new version in a sec.
pdr, can you please take a look at the ImageBuffer.cpp change?
On 2014/09/12 15:32:31, Daniel Bratell wrote: > Ah, pdr it is. I'll upload a new version in a sec. I mentioned pdr@ since he might still have comments.
On 2014/09/12 15:32:01, Daniel Bratell wrote:
> There will be (at least) one copy too much at peak still. In total we have
> 1. raw image (can't be let go).
> 2. jpeg/png encoded image (needed for step 3)
> 3. base64 encoded jpeg/png encoded image (needed for step 4)
> 4. data url string.
correct
> Optimally it would be something like (1) -> (1, 2) -> (1, 2, 3) -> (1, 3) ->
(1,
> 3, 4) -> (1, 4).
> After the last patch it will be (1) -> (1, 2) -> (1, 2, 3) -> (1, 2, 3, 4) ->
> (1, 2, 4) -> (1, 4)
Optimally, one could call
return base64Encode("data:" + mimeType + ";base64,", encodedImage);
to obtain RVO (return-value-optimization) on most modern compilers. That'd
require more extensive changes and patches with tests.
> As for memory, I think in many cases the problem can be consecutive address
> space. If you have allocated yourself a 1 GB heap and used it a while, it
might
> be hard to find 3-5 empty free blocks in the address space. It's also possible
> that the bug is something else, or that it's not 20 MB blocks but 200 MB
blocks.
That's the point of focussing on the bug in hand, I guess: leaves out
conjecture.
> I just noticed that you are not listed as OWNER for this code so I might have
> disturbed the wrong person. Sorry about that.
no problem - I wrote that code, so I am happy can review it. Your changes to it
seem fine to me.
LGTM
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/559103002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 181925 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
