1065818 - Clean up memory reporters and use of decoded size for image cache entries in imagelib

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

It just doesn't make sense to consider decoded size for image cache management. The decoded size is dynamic and changes based on all sorts of factors. We already have a place in imagelib, the surface cache, which centralizes management of decoded surfaces and has plenty of knobs to tweak. Additionally, we are likely to eventually merge the image cache with the necko cache, and the necko cache certainly won't be considering decoded size when managing itself.

It's worth making this change now because all decoded data is moving into the surface cache (see bug 977459 and its blockers). Changing it has the very nice side effect of making imagelib's memory reporters more sane, since normally memory reporters are supposed to return 0 on platforms where MallocSizeOf doesn't work. We can make imagelib's memory reporters (imgFrame's, essentially, in reality) behave this way, and get rid of the ugly "WithComputedFallbackIfHeap" method suffixes that we're currently using to document the fact that imagelib's memory reporters don't have normal behavior.

Comment 1

[Seth Fowler [:seth] [:s2h]]

It turned out that the memory reporter cleanup is really dominates this patch, so I've retitled the bug accordingly.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: Clean up memory reports and use of decoded size for image cache entries

Here's the patch. This really does several interrelated things:

1. Stop considering decoded size for image cache entries in imagelib. From now on, we consider only source size, because decoded data lives in the surface cache. That allows us to...

2. Greatly simplify the memory reporting methods on the Image class. Which works out very well, but it forces us to...

3. Do some serious refactoring of the main imagelib memory reporting code in imgLoader. This really needed doing anyway.

If you prefer, you can view this as three separate patches. Patch 1 only really involves imgRequest. Patch 2 involves Image and all of its subclasses. Patch 3 involves imgLoader. However, since they need each other to work correctly, and the total amount of code isn't that large except in imgLoader, I haven't bothered separating them. If you feel like that'd be helpful, let me know.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8487682 [details] [diff] [review]
Clean up memory reports and use of decoded size for image cache entries

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

Very nice clean-up! Great to see so much ugly code removed.

I have one major concern. At the moment, image measurements are reported in two ways:
- In the "explicit" tree there is the per-image measurements.
- In the "Other Measurements" section there is aggregate measurements, e.g.:

> 1,917,576 B (100.0%) -- images
> ├──1,255,904 B (65.49%) -- chrome
> │  ├──1,251,616 B (65.27%) ── vector/used/documents
> │  └──────4,288 B (00.22%) ── raster/used/uncompressed-heap
> ├────656,680 B (34.25%) -- content
> │    ├──425,992 B (22.22%) ── vector/used/documents
> │    └──230,688 B (12.03%) -- raster/used
> │       ├──203,296 B (10.60%) ── raw
> │       └───27,392 B (01.43%) ── uncompressed-heap
> └──────4,992 B (00.26%) -- uncached/raster/used
>        ├──4,160 B (00.22%) ── raw
>        └────832 B (00.04%) ── uncompressed-heap

AFAICT you've removed the latter, because you removed |totalSizes| from
ReportInfoArray() but didn't replace it with anything equivalent. (I tried
applying the patch to double-check this, but I got lots of conflicts.)

If so, please reinstate it, because it's useful to have this aggregate data as
well as the per-image data (and we have similar measurements for JS and DOM
stuff).

r=me with that addressed.

::: image/src/VectorImage.cpp
@@ +365,5 @@
>    return nsIntRect::GetMaxSizedIntRect();
>  }
>  
>  size_t
> +VectorImage::SizeOfSourceWithComputedFallback(MallocSizeOf aMallocSizeOf) const

I don't see a computed fallback here. Should there be one?

@@ +374,5 @@
> +  }
> +
> +  nsWindowSizes windowSizes(aMallocSizeOf);
> +  doc->DocAddSizeOfIncludingThis(&windowSizes);
> +  return windowSizes.getTotalSize();

I sure hope this doesn't end up double-counting anything. You could do a DMD run (https://wiki.mozilla.org/Performance/MemShrink/DMD) to check.

(Oh, I see you moved this from lower down... so it's probably ok.)

::: image/src/imgLoader.cpp
@@ +57,5 @@
>  public:
>    NS_DECL_ISUPPORTS
>  
> +  NS_IMETHOD CollectReports(nsIMemoryReporterCallback* aHandleReport,
> +                            nsISupports* aData, bool aAnonymize)

I can't see what changed on this line, but while you're here, can you change nsIMemoryReporterCallback to nsIHandleReportCallback, which is now the preferred name.

(I see there are other places in this patch that could do with the same change. Please do so.)

@@ +102,5 @@
>  
>      return NS_OK;
>    }
>  
> +  static int64_t ImagesContentUsedDecodedDistinguishedAmount()

Can you leave this name unchanged? The concept of "distinguished amounts" is described in xpcom/base/nsIMemoryReporter.idl and this one feeds into the MEMORY_IMAGES_CONTENT_USED_UNCOMPRESSED telemetry value. So I prefer to keep the terminology consistent throughout if possible.

@@ +132,5 @@
> +      : uri(aURI)
> +      , imageType(aType)
> +      , source(0)
> +      , decodedHeap(0)
> +      , decodedNonheap(0)

|mFoo| field names, please.

@@ +154,5 @@
> +    nsCString uri;
> +    uint16_t imageType;
> +    size_t source;
> +    size_t decodedHeap;
> +    size_t decodedNonheap;

While you're here... can these fields be private?

@@ +246,5 @@
> +                                 : aVectorPathPrefix;
> +
> +    rv = ReportValue(aHandleReport, aData, KIND_HEAP, pathPrefix,
> +                     "source",
> +                     "Raster image compressed data and vector image documents.",

s/compressed/source/ ?

@@ +287,5 @@
> +    path.Append(aPathSuffix);
> +
> +    nsAutoCString desc(aDescription);
> +
> +    return aHandleReport->Callback(EmptyCString(), path, aKind , UNITS_BYTES,

Nit: extraneous space before comma.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Thanks for the review!

(In reply to Nicholas Nethercote [:njn] from comment #3)
> If so, please reinstate it, because it's useful to have this aggregate data
> as
> well as the per-image data (and we have similar measurements for JS and DOM
> stuff).

Sure, I'll add it back. Yesterday it seemed to me that that wasn't giving us much over the totals that we get for free from the about:memory tree, but on reflection it's true that if we want totals for source, decoded-heap, and decoded-nonheap, we need to do that separately.

It strikes me that it should be possible to do this more cleanly. After discussing the matter on IRC, I've filed bug 1066303 about making this nicer.

> I don't see a computed fallback here. Should there be one?

It's not clear to me how to write a good one for this case, unfortunately. =( I think we should have one, though. I'm discussing the matter with jwatt on IRC, but if we can't think of anything better I think returning a constant value (say 1kb? still mulling over the value) is much better than nothing. That should increase the rate at which VectorImage's get evicted significantly on such platforms, which is desirable.

> I sure hope this doesn't end up double-counting anything. You could do a DMD
> run (https://wiki.mozilla.org/Performance/MemShrink/DMD) to check.
> 
> (Oh, I see you moved this from lower down... so it's probably ok.)

My understanding is that it's OK. The SVG document is private and it doesn't get a window object or anything like that, so it shouldn't be counted elsewhere in the explicit tree. (Indeed I just checked and it looks like that's the case.)

> |mFoo| field names, please.

So this is just a dumb struct which holds some data so it can be passed around more conveniently, and it's a private implementation detail of the class it's in. The pattern I've been following for such cases is to make the fields public and avoid the 'm' prefix, because the overheads of writing getters and such in these cases just isn't worth it.

Comment 5

[Nicholas Nethercote [inactive]]

> The pattern I've been following for such cases is to make the
> fields public and avoid the 'm' prefix, because the overheads of writing
> getters and such in these cases just isn't worth it.

Public is fine, but |mFoo| is still the Mozilla style.

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> > The pattern I've been following for such cases is to make the
> > fields public and avoid the 'm' prefix, because the overheads of writing
> > getters and such in these cases just isn't worth it.
> 
> Public is fine, but |mFoo| is still the Mozilla style.

Sigh. I suppose so. I've been a real stickler for the style guide myself lately when reviewing others, so I should apply the same standards to myself.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: Clean up memory reports and use of decoded size for image cache entries

This version of the patch should address all of Nicholas's review comments.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8488487 [details] [diff] [review]
Clean up memory reports and use of decoded size for image cache entries

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

I tried and again failed to apply this patch against mozilla-inbound. What repo are you using?

The aggregate stuff still isn't right. For every notable image, ReportValue() now does two reports: one with an "explicit" path, and one slightly different with a non-"explicit" path. The second one is wrong.

Look at the output with your patch applied compared to output without your patch applied. The "images" tree in the "Other Measurements" section will be very different. It shouldn't have any individual images listed. You need something like |totalSizes| in the current code.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> I tried and again failed to apply this patch against mozilla-inbound. What
> repo are you using?

Probably just a large local patch queue with the patches from all the bugs in the dependency tree starting at bug 977459 I think.

Comment 10

[Nicholas Nethercote [inactive]]

Seth, are there any obstacles here? This is a great patch; I just want to preserve the "other measurements" behaviour. Please ask if anything is unclear.

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8488487 [details] [diff] [review]
Clean up memory reports and use of decoded size for image cache entries

Clearing review request until Nick's questions are addressed.

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8488487 [details] [diff] [review]
Clean up memory reports and use of decoded size for image cache entries

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

I just talked to Seth on IRC. The change in aggregate reporting is deliberate, so I withdraw my r-. I prefer the old-style, more compact aggregate, but I'll let Seth decide which he prefers.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: Clean up memory reports and use of decoded size for image cache entries

Alright, after some discussion on IRC, Nicholas and I decided to restore the old summary behavior instead of listing individual images in the summary, as the previous patch did. This (massively changed) version of the patch implements that. The behavior should now be identical to the old imagelib memory reporting code.

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8497916 [details] [diff] [review]
Clean up memory reports and use of decoded size for image cache entries

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

Thank you!

::: image/src/VectorImage.cpp
@@ +380,5 @@
> +  if (windowSizes.getTotalSize() == 0) {
> +    // MallocSizeOf fails on this platform. Because we also use this method for
> +    // determining the size of cache entries, we need to return something
> +    // reasonable here. Unfortunately, there's no way to estimate the document's
> +    // size accurately, so we just use a constant value of 100KB, which will

I sure hope there's no way that getTotalSize() could be zero other than if MallocSizeOf fails on this platform...

::: image/src/imgLoader.cpp
@@ +193,5 @@
> +        }
> +      } else {
> +        MOZ_CRASH("Unexpected image type");
> +      }
> +      

Trailing whitespace?

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: Clean up memory reports and use of decoded size for image cache entries

Thanks for the review, Nicholas!

I addressed the trailing whitespace issue and rebased this against the updated version of bug 923302.

Comment 16

[Nicholas Nethercote [inactive]]

tnikkel: review ping!

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8498554 [details] [diff] [review]
Clean up memory reports and use of decoded size for image cache entries

Sorry for the delay.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6db5d2f985

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4188665&repo=mozilla-inbound

Comment 20

[Seth Fowler [:seth] [:s2h]]

Repushed since that issue should hopefully now be dealt with:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac38e527317

Comment 21

[Carsten Book [:Tomcat]]

sorry had to back this out again in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=b5dda6f01770 since one of this changes caused https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4220043&repo=mozilla-inbound

Comment 22

[Seth Fowler [:seth] [:s2h]]

Attachment: Clean up memory reports and use of decoded size for image cache entries

Fix a bug in imgLoader.cpp where we were passing a MallocSizeOf function when we should've just passed nullptr.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Repushed since comment 21 should have fixed the problem:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2aefa543034

Comment 24

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #23)
> Repushed since comment 21 should have fixed the problem:

I mean comment 22.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2aefa543034