Closed Bug 590790 Opened 9 years ago Closed 5 years ago

Add memory reporter for VectorImage's SVGDocumentWrapper's document

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: dholbert, Assigned: jwatt)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files, 6 obsolete files)

My patch for bug 276431 includes a quite naive & optimistic implementation for VectorImage::GetDataSize() -- it just returns sizeof(*this) for now.

Depending on how much we care about the return-value, we can definitely do better than that.

(though maybe that method matters most for decode-on-draw, with discarding images based on size & then redecoding them later -- if that's the case, maybe it's less of an issue, since VectorImage doesn't support decode-on-draw?)
Depends on: 276431
(In reply to comment #0)
> (though maybe that method matters most for decode-on-draw, with discarding
> images based on size & then redecoding them later -- if that's the case, maybe
> it's less of an issue, since VectorImage doesn't support decode-on-draw?)

GetDataSize() isn't used for decode-on-draw - it's used for the image cache.

What's the plan for SVG images and the image cache anyway?
The plan is for GetData() to traverse all a VectorImage's helper-objects (in particular, the DOM of the helper document) and return the size of all that.

(IIUC, from talking to bholley on IRC just now, I think that's the only plan that's needed, with respect to VectorImage & the image cache.)
OS: Linux → All
Hardware: x86_64 → All
My patch in bug 601470 actually promotes GetDataSize() to live on the "Image" superclass, but it depends on the GetDecodedDataSize() method (added to VectorImage in that bug).

I'm updating the summary of this bug to be about fixing GetDecodedDataSize() now. (basically just the same thing)
Summary: Make VectorImage::GetDataSize more intelligent → Make VectorImage::GetDecodedDataSize more intelligent
(Bug 664659 renamed GetDecodedDataSize to GetDecodedHeapSize - updating bug title)
Summary: Make VectorImage::GetDecodedDataSize more intelligent → Make VectorImage::GetDecodedHeapSize more intelligent
Blocks: DarkMatter
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached file about:memory contents (obsolete) —
This is my about:memory when viewing https://dl.dropbox.com/u/2128410/linker-size.svg
Geoff: This bug is about an API used for SVG-as-an-image (e.g. in <img> and CSS backgrounds) -- not for SVG documents in general.  It's not related to anything that happens when you view an SVG file directly, as it sounds like you were doing in comment 5.

So I don't think comment 5 has any bearing on this bug.

In other news, this function was renamed again since Comment 4, now to HeapSizeOfDecodedWithComputedFallback, in bug 711901. Updating summary.
Summary: Make VectorImage::GetDecodedHeapSize more intelligent → Make VectorImage::HeapSizeOfDecodedWithComputedFallback more intelligent
Attachment #620502 - Attachment is obsolete: true
While working on this it was useful to figure out how the memory reporting works for images. For my own future reference here's a summary:

imgLoader::GlobalInit() creates an imgMemoryReporter singleton and passes that to RegisterStrongMemoryReporter() to register it. When nsMemoryReporterManager::GetReportsForThisProcessExtended is invoked that imgMemoryReporter singleton is one of the reporters that has CollectReports() called on it. imgMemoryReporter::CollectReports then loops over mKnownLoaders invoking imgMemoryReporter::EntryAllSizes for each of the entries in the imgLoaders' mCache and mChromeCache arrays. imgMemoryReporter::EntryAllSizes gets the imgRequest from each imgLoader, gets the imgRequest's Image, and then calls HeapSizeOfSourceWithComputedFallback, HeapSizeOfDecodedWithComputedFallback and NonHeapSizeOfDecoded on the Image to account for the memory it uses.
Assignee: nobody → jwatt
Note to self: ImageResource::SizeOfData calls HeapSizeOfDecodedWithComputedFallback to determine the size of the cache, but the cache doesn't store the SVGDocument...
Attached patch WIP (obsolete) — Splinter Review
Currently this just attributes the SVGDocument memory to images/content/used/uncompressed-heap, but maybe we should attribute it to something else so we can distinguish image memory due to vector images from other image memory.
Blocks: 999931
When I run http://ricaud.me/mozilla/svg-benchmark/_index.html in Firefox with this patch the memory report goes from:

94,006,520 B (39.92%) ── heap-unclassified
4,308,688 B (01.83%) -- images
8,712,960 B ── imagelib-surface-cache

to:

53.79 MB (24.92%) ── heap-unclassified
42.57 MB (19.73%) -- images
8.31 MB ── imagelib-surface-cache
> Currently this just attributes the SVGDocument memory to
> images/content/used/uncompressed-heap, but maybe we should attribute it to
> something else so we can distinguish image memory due to vector images from
> other image memory.

Yes please! Maybe have a top-level "vector-images" tree? Or something else, if you can think of something better.
Attachment #8418411 - Flags: review?(n.nethercote)
Attachment #8416834 - Attachment is obsolete: true
Attachment #8418433 - Flags: review?(dholbert)
Attachment #8418433 - Flags: review?(n.nethercote)
Attachment #8418433 - Flags: review?(seth)
Comment on attachment 8418433 [details] [diff] [review]
part 2 - Add memory reporter for VectorImage's SVGDocumentWrapper's document

>Bug 590790 part 2 - Add memory reporter for VectorImage'sSVGDocumentWrapper's document. r=dholbert, r=njn

Needs a space after "VectorImage's"

>+MOZ_DEFINE_MALLOC_SIZE_OF(WindowsMallocSizeOf);
>+
>+size_t
>+VectorImage::HeapSizeOfVectorImageDocuments() const
>+{
>+  nsWindowSizes windowSizes(WindowsMallocSizeOf);
>+  mSVGDocumentWrapper->GetDocument()->DocAddSizeOfExcludingThis(&windowSizes);
>+  return windowSizes.GetTotalSize();
>+}

mSVGDocumentWrapper->GetDocument() is allowed to return null on failure (e.g. if the source data is bogus, IIRC), so we need to null-check it here.

Probably best to store it in a local variable, null-check it (and return 0 if it's null), and *then* declare windowSizes & call DocAddSizeOfExcludingThis.

Other than that, this looks good to me, modulo the fact that I don't know how memory reporting code works. :) (And per IRC, I'm not an imagelib peer so I can't really sign off on stuff outside of VectorImage; thanks for adding seth to the r? list.)
Attachment #8418433 - Flags: review?(dholbert) → review+
Comment on attachment 8418433 [details] [diff] [review]
part 2 - Add memory reporter for VectorImage's SVGDocumentWrapper's document

Review of attachment 8418433 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

We discussed on IRC that there are many crazy things about how all this works, but none of them have to be fixed in this bug.
Attachment #8418433 - Flags: review?(seth) → review+
Attachment #8418433 - Attachment is obsolete: true
Attachment #8418433 - Flags: review?(n.nethercote)
Attachment #8418476 - Flags: review?(n.nethercote)
Now reporting individual doc URIs as discussed on IRC.
Attachment #8418476 - Attachment is obsolete: true
Attachment #8418476 - Flags: review?(n.nethercote)
Attachment #8418485 - Flags: review?(n.nethercote)
Comment on attachment 8418411 [details] [diff] [review]
part 1 - Add a nsWindowSizes::GetTotalSize method

Review of attachment 8418411 [details] [diff] [review]:
-----------------------------------------------------------------

nsArenaMemoryStats and nsWindowSizes are each defined in terms of a higher-order FOR_EACH_SIZE macro, which makes implementing this kind of function less error-prone and more future-proof. Please use them. Both classes have an addToTabSizes() function that you can use as a guide.

::: dom/base/nsWindowMemoryReporter.cpp
@@ +39,5 @@
> +    mDOMCDATANodesSize +
> +    mDOMCommentNodesSize +
> +    mDOMEventTargetsSize +
> +    mDOMEventTargetsCount +
> +    mDOMEventListenersCount +

For example, nsWindowSizes includes a combination of sizes (where the unit is "bytes") and counts (which are unitless) and you're adding them all as if they're of the same unit. Using FOR_EACH_SIZE will avoid this problem because the counts aren't included.
Attachment #8418411 - Flags: review?(n.nethercote) → review-
Comment on attachment 8418485 [details] [diff] [review]
part 2 - Add memory reporter for VectorImage's SVGDocumentWrapper's document (r=dholbert, r=seth)

Review of attachment 8418485 [details] [diff] [review]:
-----------------------------------------------------------------

Can you include some example output? That always makes reviewing memory reporter patches easier.

::: image/src/Image.h
@@ +91,5 @@
> +   * aDocURL outparam.
> +   */
> +  virtual size_t HeapSizeOfVectorImageDocument(nsACString* aDocURL = nullptr) const {
> +    return 0;
> +  }

Can this be made pure virtual, so we can't fail to implement it?

::: image/src/VectorImage.cpp
@@ +370,1 @@
>    return 0;

Add a comment here explaining how the measurement is done by VectorImage::HeapsizeOfVectorImageDocument()?

::: image/src/imgLoader.cpp
@@ +140,5 @@
> +
> +    for (uint32_t i = 0; i < chrome.mVectorImageDocInfo.Length(); i++) {
> +      chrome.mVectorImageDocInfo[i].mURI.ReplaceChar('/', '\\');
> +      if (chrome.mVectorImageDocInfo[i].mUsed) {
> +        REPORT_VECTOR("explicit/images/chrome/used/vector-image-documents",

I don't think these measurements should be reported in the "images" sub-tree.. to me, vector image data feels very different to raster image data, and summing them doesn't make much sense. (I'd be interested to hear counter-arguments, though.) How about a "vector-images" sub-tree?

@@ +269,5 @@
>          // instead of ImagesMallocSizeOf to prevent DMD from seeing it reported
>          // twice.
>          *n += image->HeapSizeOfDecodedWithComputedFallback(moz_malloc_size_of);
>          *n += image->NonHeapSizeOfDecoded();
> +        *n += image->HeapSizeOfVectorImageDocument();

This function computes the value that shows up in telemetry as MEMORY_IMAGES_CONTENT_USED_UNCOMPRESSED. I don't think that should include vector stuff -- the "UNCOMPRESSED" part doesn't make sense. So this should be removed.
Attachment #8418485 - Flags: review?(n.nethercote) → feedback+
Attachment #8418411 - Attachment is obsolete: true
Attachment #8418642 - Flags: review?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #20)
> Can you include some example output? That always makes reviewing memory
> reporter patches easier.

556,241,984 B (100.0%) -- explicit
├──357,092,680 B (64.20%) -- images
│  ├──354,284,792 B (63.69%) -- content
│  │  ├──354,284,792 B (63.69%) -- used
│  │  │  ├──257,269,760 B (46.25%) ── uncompressed-nonheap
│  │  │  ├───85,863,368 B (15.44%) -- vector-image-documents
│  │  │  │   ├─────213,064 B (00.04%) ── (http://jwatt.org/mozilla/testcases/bug999931%20-%20svg-as-image-for-gaia/svg-benchmark/contacts.svg)
│  │  │  │   ├─────213,064 B (00.04%) ── (http://jwatt.org/mozilla/testcases/bug999931%20-%20svg-as-image-for-gaia/svg-benchmark/contacts.svg?0)
│  │  │  │   ├─────213,064 B (00.04%) ── (http://jwatt.org/mozilla/testcases/bug999931%20-%20svg-as-image-for-gaia/svg-benchmark/contacts.svg?1)
│  │  │  │   ├─────213,064 B (00.04%) ── (http://ricaud.me/mozilla/svg-benchmark/contacts.svg)
│  │  │  │   └─────211,640 B (00.04%) ── (http://tavmjong.free.fr/INKSCAPE/MANUAL/images/WEB/web_square.svg)
Attachment #8418642 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #20)
> ::: image/src/VectorImage.cpp
> @@ +370,1 @@
> >    return 0;
> 
> Add a comment here explaining how the measurement is done by
> VectorImage::HeapsizeOfVectorImageDocument()?

Why, because it's not clear why we're returning zero? It really should be when the function to get the size of the doc finds there is no doc - obviously no memory is taken up storing a non-existent doc. How about I add a comment along the lines of "No document, so no memory used"?

> ::: image/src/imgLoader.cpp
> @@ +140,5 @@
> > +
> > +    for (uint32_t i = 0; i < chrome.mVectorImageDocInfo.Length(); i++) {
> > +      chrome.mVectorImageDocInfo[i].mURI.ReplaceChar('/', '\\');
> > +      if (chrome.mVectorImageDocInfo[i].mUsed) {
> > +        REPORT_VECTOR("explicit/images/chrome/used/vector-image-documents",
> 
> I don't think these measurements should be reported in the "images"
> sub-tree.. to me, vector image data feels very different to raster image
> data, and summing them doesn't make much sense. (I'd be interested to hear
> counter-arguments, though.) How about a "vector-images" sub-tree?

I think it should still be under "images", but I'm happy to separate it. How about we have "explicit/images/raster/..." and "explicit/images/vector/..."?

> @@ +269,5 @@
> >          // instead of ImagesMallocSizeOf to prevent DMD from seeing it reported
> >          // twice.
> >          *n += image->HeapSizeOfDecodedWithComputedFallback(moz_malloc_size_of);
> >          *n += image->NonHeapSizeOfDecoded();
> > +        *n += image->HeapSizeOfVectorImageDocument();
> 
> This function computes the value that shows up in telemetry as
> MEMORY_IMAGES_CONTENT_USED_UNCOMPRESSED. I don't think that should include
> vector stuff -- the "UNCOMPRESSED" part doesn't make sense. So this should
> be removed.

I guess it depends what "UNCOMPRESSED" means. If it means decoded, then it sort of makes sense to me. I've removed it, but is there somewhere else that this should show up for telemetry?
> I think it should still be under "images", but I'm happy to separate it. 
> How about we have "explicit/images/raster/..." and
> "explicit/images/vector/..."?

This patch does that.
Attachment #8418485 - Attachment is obsolete: true
Attachment #8418666 - Flags: review?(n.nethercote)
Hmm, actually I think I'd prefer grouping chrome/content more strongly, so:

  explicit/images/{content|chrome}/{raster/vector}/...

rather than:

  explicit/images/{raster/vector}/{content|chrome}/...

Let me know if you're okay with me making that change when you review.
Comment on attachment 8418666 [details] [diff] [review]
part 2 - Add memory reporter for VectorImage's SVGDocumentWrapper's document (r=dholbert, r=seth)

Review of attachment 8418666 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this. Nice to fix a 3.5 year old bug.

::: image/src/VectorImage.cpp
@@ +362,5 @@
>    // our helper SVG document as we receive it, for it to parse.
>    // So 0 is an appropriate return value here.
> +  // If implementing this, we'll need to restructure our callers to make sure
> +  // any amount we return is attributed to the vector images measure (i.e.
> +  // "explicit/images/vector/{content|chrome}/{used/unused}/...")

Typo: you want '|' for "{used/unused}". Though I would actually write "{used,unused}" (shell style) or "(used|unused)" (regexp style).

@@ +371,5 @@
>  VectorImage::HeapSizeOfDecodedWithComputedFallback(mozilla::MallocSizeOf aMallocSizeOf) const
>  {
> +  // If implementing this, we'll need to restructure our callers to make sure
> +  // any amount we return is attributed to the vector images measure (i.e.
> +  // "explicit/images/vector/{content|chrome}/{used/unused}/...")

Ditto.

@@ +380,5 @@
>  VectorImage::NonHeapSizeOfDecoded() const
>  {
> +  // If implementing this, we'll need to restructure our callers to make sure
> +  // any amount we return is attributed to the vector images measure (i.e.
> +  // "explicit/images/vector/{content|chrome}/{used/unused}/...")

Ditto.

@@ +389,5 @@
>  VectorImage::OutOfProcessSizeOfDecoded() const
>  {
> +  // If implementing this, we'll need to restructure our callers to make sure
> +  // any amount we return is attributed to the vector images measure (i.e.
> +  // "explicit/images/vector/{content|chrome}/{used/unused}/...")

Ditto.

::: image/src/imgLoader.cpp
@@ +74,5 @@
>                                NS_LITERAL_CSTRING(_desc), closure);            \
>        NS_ENSURE_SUCCESS(rv, rv);                                              \
>      } while (0)
>  
> +    REPORT("explicit/images/raster/chrome/used/raw",

There's a comment in xpcom/base/nsIMemoryReporterManager.idl like this:

   * |imagesContentUsedUncompressed| (UNITS_BYTES)  Memory used for decoded
   * images in content.

Can you change "decoded images" to "decoded raster images"?

(If you're grimacing at this far-flung interaction, I can sympathize. But at least it's only a comment -- a while ago, before I made some things better, you would have had to change some code around here as well in order to not break telemetry!)

@@ +124,5 @@
>             "Memory used by not in-use content images (uncompressed data).");
>  
>  #undef REPORT
>  
> +#define REPORT_VECTOR(_path, _uri, _kind, _amount, _desc)                     \

|_kind| is always |KIND_HEAP|, so you could hard-wire it within the macro.

@@ +137,5 @@
> +                              NS_LITERAL_CSTRING(_desc), closure);            \
> +      NS_ENSURE_SUCCESS(rv, rv);                                              \
> +    } while (0)
> +
> +    for (uint32_t i = 0; i < chrome.mVectorImageDocInfo.Length(); i++) {

Reporting image URIs is very useful, but it might bloat the output. How many of these are there likely to be? Something that the JS memory reporter does is have a threshold for case like these -- any images smaller than a certain size get aggregated into a single aggregate entry. Would that be worthwhile?
Attachment #8418666 - Flags: review?(n.nethercote) → review+
> > This function computes the value that shows up in telemetry as
> > MEMORY_IMAGES_CONTENT_USED_UNCOMPRESSED. I don't think that should include
> > vector stuff -- the "UNCOMPRESSED" part doesn't make sense. So this should
> > be removed.
> 
> I guess it depends what "UNCOMPRESSED" means. If it means decoded, then it
> sort of makes sense to me. I've removed it, but is there somewhere else that
> this should show up for telemetry?

Yeah, "uncompressed" means "decoded". No need for this vector stuff to show up in telemetry; we only have a handful of memory measurements done for telemetry, and vector images aren't common enough to warrant adding something new, IMO.
(In reply to Jonathan Watt [:jwatt] from comment #25)
> Hmm, actually I think I'd prefer grouping chrome/content more strongly, so:
> 
>   explicit/images/{content|chrome}/{raster/vector}/...
> 
> rather than:
> 
>   explicit/images/{raster/vector}/{content|chrome}/...

Sounds fine.
(In reply to Nicholas Nethercote [:njn] from comment #26)
> (If you're grimacing at this far-flung interaction

Nope. Out of date comments are way to common and suck. Glad you noticed this.

> Reporting image URIs is very useful, but it might bloat the output. How many
> of these are there likely to be? Something that the JS memory reporter does
> is have a threshold for case like these -- any images smaller than a certain
> size get aggregated into a single aggregate entry. Would that be worthwhile?

SVG-as-an-image isn't all that common so I'd expect this not to be an issue for a while. I think it's probably good to report them for now and if it becomes an issue we can add a threshold without much difficulty.
(In reply to Jonathan Watt [:jwatt] from comment #29)
> (In reply to Nicholas Nethercote [:njn] from comment #26)
> > (If you're grimacing at this far-flung interaction
> 
> Nope. Out of date comments are way to common and suck. Glad you noticed this.

The naming of those functions is now slightly incorrect, but it's probably not worth changing.

> SVG-as-an-image isn't all that common so I'd expect this not to be an issue
> for a while. I think it's probably good to report them for now and if it
> becomes an issue we can add a threshold without much difficulty.

Fair enough.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a520a43d065c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a968bedfcca
Summary: Make VectorImage::HeapSizeOfDecodedWithComputedFallback more intelligent → Add memory reporter for VectorImage's SVGDocumentWrapper's document
You need to log in before you can comment on or make changes to this bug.