Bug 158139 – SVGImage should report its memory cost to JS garbage collector

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

If an SVG takes too much memory and it is not referenced by the current document, the garbage collector will not try to free it, unless it has to (like a memory warning from the OS). This can cause the WebProcess to jetsam because it tries to exceed its memory limit.

The SVGImage needs to report an approximation for its memory cost since it is hard and slow process to count all the bytes which are referenced by all the children nodes. This calculation can be very complex if the SVG has animating nodes. If the SVGImage under estimates its memory cost, this will fix the problem partially since the garbage collector will think that the WebProcess is not taking that much memory although it could be near its limit. If the SVGImage over estimates its memory cost, the garbage collector will work more aggressively and it will try more often to free unreferenced memory although there may not be anything to release. This might cause perf and power issues. So we have to be very careful in how much we report.

[Said Abou-Hallawa]

Created attachment 279932 [details]
Patch

[WebKit Commit Bot]

Attachment 279932 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/graphics/SVGImage.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Comment on attachment 279932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279932&action=review

> Source/WebCore/svg/graphics/SVGImage.cpp:371
> +    static const size_t MemoryCostFactor = 10;

I could not figure out where the rest of the memory are gone.  The calculated memory was almost 10% of the actual memory used by the WebProcess for the SVGImage.

> Source/WebCore/svg/graphics/SVGImage.cpp:420
> +        reportApproximateMemoryCost();

Should we make this reporting only for iOS?

[Said Abou-Hallawa]

The bug is fixed on WK1 but not for WK2 unless I multiply the memory cost of the SVGImage by a large factor (4-10). I need to investigate which process in WK2 allocates the extra memory and for what purpose.

[Said Abou-Hallawa]

Created attachment 280204 [details]
Patch

[WebKit Commit Bot]

Attachment 280204 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/graphics/SVGImage.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

The remaining memory not accounted for here is the render tree in the SVG image.

We could have each renderer report its size, like this patch has each DOM node report its size, but that's a very intrusive solution and it's likely to become stale over time. Another approach is to estimate size of render tree based on size of DOM. But as long as we're going to estimate, we might as well do something even simpler: Just take the (uncompressed) encoded size of the SVG image and multiply by a constant scaling factor.

Every decoded image has some average decompression ratio relative to its encoded size. SVG image happens to have a 10X-100X ratio because SVG is such a crazy image format. That's why these simple images can trigger jetsam. But it's easy enough for us to measure a bunch of SVG images, compute an average decompression ratio, and apply that average to all images. The more complex estimation is still just an estimation, and not necessarily any more accurate.

Side note: We should also arrange for SVG images to honor our existing WebCore::MemoryCache mechanism for throwing away decoded image data on demand.

[Said Abou-Hallawa]

(In reply to comment #7)
> The remaining memory not accounted for here is the render tree in the SVG
> image.
> 
> We could have each renderer report its size, like this patch has each DOM
> node report its size, but that's a very intrusive solution and it's likely
> to become stale over time. Another approach is to estimate size of render
> tree based on size of DOM. But as long as we're going to estimate, we might
> as well do something even simpler: Just take the (uncompressed) encoded size
> of the SVG image and multiply by a constant scaling factor.
> 

In SVGPolyElement::approximateMemoryCost() and SVGPathElement::approximateMemoryCost(), I added the size of the most expensive renderer: the RenderSVGPath. I also add an estimate for the associated Path object. It is not the perfect implementation but as you said having a cleaner and more accurate reporting for the renderers will be very intrusive.

> Every decoded image has some average decompression ratio relative to its
> encoded size. SVG image happens to have a 10X-100X ratio because SVG is such
> a crazy image format. That's why these simple images can trigger jetsam. But
> it's easy enough for us to measure a bunch of SVG images, compute an average
> decompression ratio, and apply that average to all images. The more complex
> estimation is still just an estimation, and not necessarily any more
> accurate.
> 

I will investigate this option but I don't think the ratio can be calculated easily or generalized. Unlike bitmap images, SVG images have the concept of defining resources and reusing them over and over. For every use element a shadow tree is cloned from the target resource. The shadow elements takes as much memory as the target elements. The shadow elements have to create new renderers as well. So although the encoded data might have a line like this:

<use xlink:href="#SomeTarget"/>

The SVGImage decoder might have to be build big DOM and render trees for this markup line.

> Side note: We should also arrange for SVG images to honor our existing
> WebCore::MemoryCache mechanism for throwing away decoded image data on
> demand.

I filed https://bugs.webkit.org/show_bug.cgi?id=158264 to track this issue.

[Said Abou-Hallawa]

Created attachment 280243 [details]
Patch

[Tim Horton]

Comment on attachment 280243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280243&action=review

Geoff should give this its real review.

> Source/WebCore/svg/SVGPathElement.cpp:370
> +    // We need to count for the memory which is allocated by the RenderSVGPath::m_path.

s/count/account/

> Source/WebCore/svg/SVGPolyElement.cpp:136
> +    // We need to count for the memory which is allocated by the RenderSVGPath::m_path.

Ditto.

> Source/WebCore/svg/graphics/SVGImage.cpp:363
> +    size_t decodeImagedMemoryCost = 0;

Imaged?

> Source/WebCore/svg/graphics/SVGImage.cpp:371
> +    // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
> +    // https://bugs.webkit.org/show_bug.cgi?id=142595

Don't know anything about this mechanism, but it seems weird that we're adding new calls to deprecated functions.

[Said Abou-Hallawa]

Created attachment 280248 [details]
Patch

[Geoffrey Garen]

Since we really want to fix this now, and this patch helps and does not hurt, I think it's OK to land this patch as-is with Tim's corrections. But a lot of follow-up is required:

(1) You need to adopt reportExtraMemoryAllocated + reportExtraMemoryVisited;

(2) You need to adopt setDecodedSize() + destroyDecodedData() in SVGImage and SVGImageForContainer.

(3) There are SVG elements that can have arbitrary size that aren't counted in this patch. So, this patch only works for a limited set of images.

For example, SVGSMILElement has vectors of conditions, begin times, and end times. There's also SVGAttributeToPropertyMap. SVGRenderStyle keeps a vector of SVGLength. SVGInlineTextBox has a vector of SVGTextFragment. SVGMarkerData has a vector of MarkerPosition. SVGSubPathData has a vector of FloatPoint. I have about 1000 more lines of find results to go through just to list the vectors -- and that doesn't cover any other kind of collection.

I don't think it's practical to exhaustively list all possible collections in all possible SVG data structures. This is why I prefer the constant scaling factor approach: all of these collections depend initially on some listing in the source text.

You mention that a scaling factor doesn't work for use elements. That's a good point. Let's multiply size of image by number of use elements.

Still, if you prefer the exhaustive approach, you can take that approach to solve (3). But we can't leave (3) unsolved.

[Said Abou-Hallawa]

Comment on attachment 280243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280243&action=review

>> Source/WebCore/svg/SVGPathElement.cpp:370
>> +    // We need to count for the memory which is allocated by the RenderSVGPath::m_path.
> 
> s/count/account/

Fixed

>> Source/WebCore/svg/SVGPolyElement.cpp:136
>> +    // We need to count for the memory which is allocated by the RenderSVGPath::m_path.
> 
> Ditto.

Fixed

>> Source/WebCore/svg/graphics/SVGImage.cpp:363
>> +    size_t decodeImagedMemoryCost = 0;
> 
> Imaged?

Name was changed to decodedImageMemoryCost.

>> Source/WebCore/svg/graphics/SVGImage.cpp:371
>> +    // https://bugs.webkit.org/show_bug.cgi?id=142595
> 
> Don't know anything about this mechanism, but it seems weird that we're adding new calls to deprecated functions.

I looked for reportExtraMemoryAllocated in the source code and I found it is rarely used outside JavaScriptCore. I tried the following code to do the reporting:

    JSC::JSLockHolder lock(document->vm());
    document->vm().heap.reportExtraMemoryAllocated(decodedImageMemoryCost + data()->size());

And also the following code

    JSC::JSLockHolder lock(document->topDocument().vm());
    document->topDocument().vm().heap.reportExtraMemoryAllocated(decodedImageMemoryCost + data()->size());

But these changes did not work correctly as deprecatedReportExtraMemory() does. I think there is still something missing related to reportExtraMemoryAllocated(). Otherwise all the FIXME comments should have been removed and deprecatedReportExtraMemory() was deleted .

[WebKit Commit Bot]

Comment on attachment 280248 [details]
Patch

Clearing flags on attachment: 280248

Committed r201561: <http://trac.webkit.org/changeset/201561>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

Said, should we have other bugs covering Geoff's points?

[Said Abou-Hallawa]

(In reply to comment #12)
> Since we really want to fix this now, and this patch helps and does not
> hurt, I think it's OK to land this patch as-is with Tim's corrections. But a
> lot of follow-up is required:
> 
> (1) You need to adopt reportExtraMemoryAllocated + reportExtraMemoryVisited;
> 

I tried something similar the fix of https://bugs.webkit.org/show_bug.cgi?id=142595 but it did not work as expected. Maybe I was not getting the correct scriptExecutionContext() to get the heap from. See my comment below https://bugs.webkit.org/show_bug.cgi?id=158139#c13. By the way there are ten places in the code which are still using deprecatedReportExtraMemory().

I filed https://bugs.webkit.org/show_bug.cgi?id=158274 to track the switch from deprecatedReportExtraMemory() to reportExtraMemoryAllocated()+reportExtraMemoryVisited().

> (2) You need to adopt setDecodedSize() + destroyDecodedData() in SVGImage
> and SVGImageForContainer.
> 

This is tracked by https://bugs.webkit.org/show_bug.cgi?id=158264.

> (3) There are SVG elements that can have arbitrary size that aren't counted
> in this patch. So, this patch only works for a limited set of images.
> 
> For example, SVGSMILElement has vectors of conditions, begin times, and end
> times. There's also SVGAttributeToPropertyMap. SVGRenderStyle keeps a vector
> of SVGLength. SVGInlineTextBox has a vector of SVGTextFragment.
> SVGMarkerData has a vector of MarkerPosition. SVGSubPathData has a vector of
> FloatPoint. I have about 1000 more lines of find results to go through just
> to list the vectors -- and that doesn't cover any other kind of collection.
> 
> I don't think it's practical to exhaustively list all possible collections
> in all possible SVG data structures. This is why I prefer the constant
> scaling factor approach: all of these collections depend initially on some
> listing in the source text.
> 
> You mention that a scaling factor doesn't work for use elements. That's a
> good point. Let's multiply size of image by number of use elements.
> 
> Still, if you prefer the exhaustive approach, you can take that approach to
> solve (3). But we can't leave (3) unsolved.

I filed https://bugs.webkit.org/show_bug.cgi?id=158281 to track this issue.

[Said Abou-Hallawa]

(In reply to comment #16)
> Said, should we have other bugs covering Geoff's points?

Done. Three bugs were filed and attached in the See Also.