Daniel Holbert [:dholbert] Reporter
	Description • 14 years ago

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?)

	Bobby Holley (:bholley)
	Comment 1 • 14 years ago

(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?

	Daniel Holbert [:dholbert] Reporter
	Comment 2 • 14 years ago

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.)

	Daniel Holbert [:dholbert] Reporter
	Comment 3 • 14 years ago

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)

	Daniel Holbert [:dholbert] Reporter
	Comment 4 • 13 years ago

(Bug 664659 renamed GetDecodedDataSize to GetDecodedHeapSize - updating bug title)

	Geoff Lankow (:darktrojan)
	Comment 5 • 12 years ago

This is my about:memory when viewing https://dl.dropbox.com/u/2128410/linker-size.svg

	Daniel Holbert [:dholbert] Reporter
	Comment 6 • 11 years ago

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.

	Jonathan Watt [:jwatt] Assignee
	Comment 7 • 10 years ago

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.

	Jonathan Watt [:jwatt] Assignee
	Comment 8 • 10 years ago

Note to self: ImageResource::SizeOfData calls HeapSizeOfDecodedWithComputedFallback to determine the size of the cache, but the cache doesn't store the SVGDocument...

	Jonathan Watt [:jwatt] Assignee
	Comment 10 • 10 years ago

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.

	Jonathan Watt [:jwatt] Assignee
	Comment 11 • 10 years ago

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

	Nicholas Nethercote [inactive]
	Comment 12 • 10 years ago

> 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.

	Daniel Holbert [:dholbert] Reporter
	Comment 15 • 10 years ago

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.)

	Seth Fowler [:seth] [:s2h]
	Comment 16 • 10 years ago

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.

	Jonathan Watt [:jwatt] Assignee
	Comment 18 • 10 years ago

Now reporting individual doc URIs as discussed on IRC.

	Nicholas Nethercote [inactive]
	Comment 19 • 10 years ago

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.

	Nicholas Nethercote [inactive]
	Comment 20 • 10 years ago

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.

	Jonathan Watt [:jwatt] Assignee
	Comment 22 • 10 years ago

(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)

	Jonathan Watt [:jwatt] Assignee
	Comment 23 • 10 years ago

(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?

	Jonathan Watt [:jwatt] Assignee
	Comment 24 • 10 years ago

> 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.

	Jonathan Watt [:jwatt] Assignee
	Comment 25 • 10 years ago

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.

	Nicholas Nethercote [inactive]
	Comment 26 • 10 years ago

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?

	Nicholas Nethercote [inactive]
	Comment 27 • 10 years ago

> > 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.

	Nicholas Nethercote [inactive]
	Comment 28 • 10 years ago

(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.

	Jonathan Watt [:jwatt] Assignee
	Comment 29 • 10 years ago

(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.

	Nicholas Nethercote [inactive]
	Comment 30 • 10 years ago

(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.

	Jonathan Watt [:jwatt] Assignee
	Comment 31 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a520a43d065c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a968bedfcca

	Jonathan Watt [:jwatt] Assignee
	Comment 32 • 10 years ago

Plus follow-up:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cdfd0b709c5

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a520a43d065c
https://hg.mozilla.org/mozilla-central/rev/8a968bedfcca
https://hg.mozilla.org/mozilla-central/rev/9cdfd0b709c5