1370412 - Excessive CPU usage by the content process at lumi.ava01.com

Status: RESOLVED FIXED

(Core :: Graphics, enhancement, P3)

Description

[Mantaroh Yoshinaga[:mantaroh]]

Some tweets on twitter was reported the excessive CPU usage of Firefox.

If we open this site[1], CPU usage of Firefox is higher than other UAs.
CPU usage of Nightly is around 300% on my macOS. (Windows 10 is around 95%.)
[1] http://lumi.ava01.com/   (VOCALOID product's page)

On macOS, the CPU usage of Chromium and Safari TP is around 100%, and on WIndows, Edge is around 15%.

Futhermore, this site use transform(scale) on top image, however it couldn't render correctly.

Profiler result is here: https://perfht.ml/2rYaJSa

It looks like consumed most of time for painting. But I'm not sure that why it caused this phenomenon.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This site seems to cause 100% cpu in the process even after closing the window in nightly 55.  about:memory did not show the window any more.

Comment 2

[Chris Peterson [:cpeterson]]

QRC_NeedAnalysis of profile in comment 0

Comment 3

[Markus Stange [:mstange]]

This site uses two large sprite images: https://lumi.ava01.com/assets/sprite/src-lumi-s742fd468da.png and https://lumi.ava01.com/assets/sprite/src-s2323c79e1b.png .

There are two problems here:
 (1) We're painting these images on the main thread, and
 (2) our downscale-during-decode job queue gets backed up.

These images are used as CSS background images on <span> elements, which are wrapped in <div> elements with animated transforms.
We don't create ImageLayers for them because they're clipped background images, so we paint them on the main thread.

However, more importantly, even after I've closed the tab, our image decoding threads are busy with this call stack:

nsThread::ProcessNextEvent(bool, bool*)
mozilla::image::DecodePoolWorker::Run()
mozilla::image::DecodedSurfaceProvider::Run()
mozilla::image::Decoder::Decode(mozilla::image::IResumable*)
mozilla::image::nsPNGDecoder::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)
mozilla::Maybe<[...]>([...])
mozilla::image::nsPNGDecoder::ReadPNGData(char const*, unsigned long)
MOZ_PNG_process_data
MOZ_PNG_push_read_IDAT
MOZ_PNG_proc_IDAT_data

It looks like the decode pool gets swamped and never recovers.

Comment 4

[Markus Stange [:mstange]]

I cut off that stack prematurely; here's the rest of it:

MOZ_PNG_push_proc_row
mozilla::image::nsPNGDecoder::WriteRow(unsigned char*)
mozilla::image::DownscalingFilter<mozilla::image::SurfaceSink>::DoAdvanceRow()
void skia::ConvolveHorizontally<true>(unsigned char const*, skia::ConvolutionFilter1D const&, unsigned char*)

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

It is constantly requesting a new downscale on decode for a different size, and we don't attempt to throttle it in anyway. We should probably accept a substitute when there are too many outstanding decodes.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Also, even slowing down the rate of outstanding decodes doesn't help with the memory pressure we are going to encounter. I easily reached 1GB of decoded surfaces on this page for a single image at various sizes. That might not be the best use of resources ;).

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Began work on a factor 2 algorithm to minimize the number of decodes and CPU usage in this sort of situation:

1) Two preferences will control whether we go into "factor 2" mode:
   a) Must exceed the given maximum number of decoded surfaces for a particular image, and
   b) Must exceed the given maximum estimated memory required for the aggregate surfaces

When not in "factor 2" mode:

2) Proceed as we do today.

When in "factor 2" mode:

3) Next time we need to decode to a particular size, we will instead decode to a "factor 2" size of the largest, native size of the image, which is the "best match" (as determined by the same logic in SurfaceCache::LookupBestMatch) to the requested size.

4) When a decode completes, we will attempt to compact the surface cache to eliminate all previously decoded surfaces which are not a "factor 2" sizes. They can only be removed when their "best match factor 2" size is already in the cache.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add preference to control "factor of 2" mode for ImageSurfaceCache., v1

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Give image::LookupResult an optional preferred size to decode at when the surface is not found., v1

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Break out ImageSurfaceCache::CompareArea from LookupBestMatch for reuse., v1

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state., v1

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add ImageSurfaceCache factor of 2 mode size calculations., v1

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. ImageSurfaceCache::LookupBestMatch should enter factor of 2 mode on cache misses., v1

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 7. ImageSurfaceCache::Lookup should enter factor of 2 mode on cache misses., v1

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8. Add ImageSurfaceCache::Prune to discard surfaces which are not needed in factor of 2 mode., v1

first try attempt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66badc3f7a110f196139f0171acb09a29d06ba1a

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8. Add ImageSurfaceCache::Prune to discard surfaces which are not needed in factor of 2 mode., v2

Fix how I did not call SurfaceCacheImpl::StopTracking for pruned surfaces. This caused all sorts of troublesome issues.

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 9. Add factor of 2 state to the SurfaceCache memory reports., v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1038a0ea2ded62bd5f4e5831c2573874e013d66

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 0. Add imgIContainer::GetNativeSizesLength to determine a ceiling on the maximum number of expected, unique surfaces., v1

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add preference to control "factor of 2" mode for ImageSurfaceCache., v2

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state., v2

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add ImageSurfaceCache factor of 2 mode size calculations., v2

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. ImageSurfaceCache::LookupBestMatch should enter factor of 2 mode on cache misses., v2

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 7. ImageSurfaceCache::Lookup should enter factor of 2 mode on cache misses., v2

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8a. Add "explicit" flag to ISurfaceProvider state to indicate when the caller won't accept substitutes., v1

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8. Add ImageSurfaceCache::Prune to discard surfaces which are not needed in factor of 2 mode., v3

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 9. Add factor of 2 state to the SurfaceCache memory reports., v2

Comment 27

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 10. Add SurfaceCache gtests for factor of 2 mode., v1

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 9. Add factor of 2 state to the SurfaceCache memory reports., v3

Fix a build warning/error.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2a379ee254504191abfd21a6572e38037184a98

Note this appears to fix several other related bugs from my testing (and can trigger the factor of 2 mode). Namely bug 1329377, bug 1243446 and bug 1157559.

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrew Osmond [:aosmond] from comment #28)
> Created attachment 8880173 [details] [diff] [review]
> Part 9. Add factor of 2 state to the SurfaceCache memory reports., v3
> 
> Fix a build warning/error.
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b2a379ee254504191abfd21a6572e38037184a98
> 
> Note this appears to fix several other related bugs from my testing (and can
> trigger the factor of 2 mode). Namely bug 1329377, bug 1243446 and bug
> 1157559.

None of the failures come across as related. I doubt talos tests are affected, but who knows, maybe it worked some magic:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda27cb3a80dd782c9efd5c5ee15285d4b9dd0b3
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=fda27cb3a80dd782c9efd5c5ee15285d4b9dd0b3

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 0. Add imgIContainer::GetNativeSizesLength to determine a ceiling on the maximum number of expected, unique surfaces., v2

Recent try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c64046493c529697616654b92d1c77ce0816664b

Comment 31

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8877256 [details] [diff] [review]
Part 2. Give image::LookupResult an optional preferred size to decode at when the surface is not found., v1

> enum class MatchType : uint8_t
> {
>   NOT_FOUND,  // No matching surface and no placeholder.
>   PENDING,    // Found a matching placeholder, but no surface.
>   EXACT,      // Found a surface that matches exactly.
>   SUBSTITUTE_BECAUSE_NOT_FOUND,  // No exact match, but found a similar one.
>-  SUBSTITUTE_BECAUSE_PENDING     // Found a similar surface and a placeholder
>+  SUBSTITUTE_BECAUSE_PENDING,    // Found a similar surface and a placeholder
>                                  // for an exact match.
>+  SUBSTITUTE_BECAUSE_BEST        // No exact match, but this is the best we
>+                                 // are able to decode.
> };

Can you explain here why we would hit this? That is probably more useful to callers trying to determine how to handle this return value.

Have you checked the places that use the current values in the enum to see if they need to be changed because of this new value? Say because they want to check all substitute return values, so they should be modified to include this new kind of substitution.

>+  gfx::IntSize mDecodeSize;

What does this field mean?

Comment 32

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8880156 [details] [diff] [review]
Part 1. Add preference to control "factor of 2" mode for ImageSurfaceCache., v2

Can you explain what native sizes means in the pref file?

Comment 33

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8880160 [details] [diff] [review]
Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state., v2

>+ * The cache may also enter "factor of 2" mode which occurs when the number of
>+ * surfaces in the cache exceeds the "image.cache.factor2.threshold-surfaces"
>+ * pref and the aggregate size of the surfaces exceeds the
>+ * "image.cache.factor2.threshold-kb" pref. When in "factor of 2" mode, the

I don't see a image.cache.factor2.threshold-kb pref in any of the other patches.

Comment 34

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8880160 [details] [diff] [review]
Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state., v2

>+    // Determine how many native surfaces this image has. Zero means we either
>+    // don't know, or we don't want to limit the number of surfaces for this
>+    // image arbitrarily.

Why does 0 native sizes mean that? Why would we not want to limit the number of surfaces for one image?

Comment 35

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8880161 [details] [diff] [review]
Part 5. Add ImageSurfaceCache factor of 2 mode size calculations., v2

Do we actually need to store an array for this? Can't we just compute the values on demand when we need them?

Comment 36

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #31)
> Comment on attachment 8877256 [details] [diff] [review]
> Part 2. Give image::LookupResult an optional preferred size to decode at
> when the surface is not found., v1
> 
> > enum class MatchType : uint8_t
> > {
> >   NOT_FOUND,  // No matching surface and no placeholder.
> >   PENDING,    // Found a matching placeholder, but no surface.
> >   EXACT,      // Found a surface that matches exactly.
> >   SUBSTITUTE_BECAUSE_NOT_FOUND,  // No exact match, but found a similar one.
> >-  SUBSTITUTE_BECAUSE_PENDING     // Found a similar surface and a placeholder
> >+  SUBSTITUTE_BECAUSE_PENDING,    // Found a similar surface and a placeholder
> >                                  // for an exact match.
> >+  SUBSTITUTE_BECAUSE_BEST        // No exact match, but this is the best we
> >+                                 // are able to decode.
> > };
> 
> Can you explain here why we would hit this? That is probably more useful to
> callers trying to determine how to handle this return value.
> 

Yes will update.

> Have you checked the places that use the current values in the enum to see
> if they need to be changed because of this new value? Say because they want
> to check all substitute return values, so they should be modified to include
> this new kind of substitution.
> 

Yes I did. There are not many users of LookupBestMatch.

> >+  gfx::IntSize mDecodeSize;
> 
> What does this field mean?

mDecodeSize is the size the SurfaceCache is suggesting the caller use when decoding (e.g. downscale-on-decode) due to returning NOT_FOUND or SUBSTITUTE_BECAUSE_NOT_FOUND.

(In reply to Timothy Nikkel (:tnikkel) from comment #32)
> Comment on attachment 8880156 [details] [diff] [review]
> Part 1. Add preference to control "factor of 2" mode for ImageSurfaceCache.,
> v2
> 
> Can you explain what native sizes means in the pref file?

Yes will update.

(In reply to Timothy Nikkel (:tnikkel) from comment #33)
> Comment on attachment 8880160 [details] [diff] [review]
> Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state., v2
> 
> >+ * The cache may also enter "factor of 2" mode which occurs when the number of
> >+ * surfaces in the cache exceeds the "image.cache.factor2.threshold-surfaces"
> >+ * pref and the aggregate size of the surfaces exceeds the
> >+ * "image.cache.factor2.threshold-kb" pref. When in "factor of 2" mode, the
> 
> I don't see a image.cache.factor2.threshold-kb pref in any of the other
> patches.

Ah yes, I decided to skip the memory usage thresholding as I deemed the surface thresholding sufficient. I will update the comments.

(In reply to Timothy Nikkel (:tnikkel) from comment #34)
> Comment on attachment 8880160 [details] [diff] [review]
> Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state., v2
> 
> >+    // Determine how many native surfaces this image has. Zero means we either
> >+    // don't know, or we don't want to limit the number of surfaces for this
> >+    // image arbitrarily.
> 
> Why does 0 native sizes mean that? Why would we not want to limit the number
> of surfaces for one image?

I should make the comments clearer. It is referring mainly to VectorImage, which always returns 0. I think the quality difference with SVG would be much more apparent to the user so I sort of punted on solving it. I don't have any evidence today that content authors are requesting many many different sizes of an SVG unreasonably.

(In reply to Timothy Nikkel (:tnikkel) from comment #35)
> Comment on attachment 8880161 [details] [diff] [review]
> Part 5. Add ImageSurfaceCache factor of 2 mode size calculations., v2
> 
> Do we actually need to store an array for this? Can't we just compute the
> values on demand when we need them?

We can. But it will need to recompute them on each lookup because typically the caller doesn't request exactly a factor-of-2 size, and so it needs the set to determine whether to return SUBSTITUTE_BECAUSE_NOT_FOUND vs SUBSTITUTE_BECAUSE_BEST/PENDING.

Comment 37

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #36)
> > >+  gfx::IntSize mDecodeSize;
> > 
> > What does this field mean?
> 
> mDecodeSize is the size the SurfaceCache is suggesting the caller use when
> decoding (e.g. downscale-on-decode) due to returning NOT_FOUND or
> SUBSTITUTE_BECAUSE_NOT_FOUND.

Can you include comments to describe this? Also the name could be clearer. mSuggestedSize?

> (In reply to Timothy Nikkel (:tnikkel) from comment #34)
> > Comment on attachment 8880160 [details] [diff] [review]
> > Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state., v2
> > 
> > >+    // Determine how many native surfaces this image has. Zero means we either
> > >+    // don't know, or we don't want to limit the number of surfaces for this
> > >+    // image arbitrarily.
> > 
> > Why does 0 native sizes mean that? Why would we not want to limit the number
> > of surfaces for one image?
> 
> I should make the comments clearer. It is referring mainly to VectorImage,
> which always returns 0. I think the quality difference with SVG would be
> much more apparent to the user so I sort of punted on solving it. I don't
> have any evidence today that content authors are requesting many many
> different sizes of an SVG unreasonably.

Wasn't there a bug recently where exactly this was happening? I'm fairly sure I remember such.

> (In reply to Timothy Nikkel (:tnikkel) from comment #35)
> > Comment on attachment 8880161 [details] [diff] [review]
> > Part 5. Add ImageSurfaceCache factor of 2 mode size calculations., v2
> > 
> > Do we actually need to store an array for this? Can't we just compute the
> > values on demand when we need them?
> 
> We can. But it will need to recompute them on each lookup because typically
> the caller doesn't request exactly a factor-of-2 size, and so it needs the
> set to determine whether to return SUBSTITUTE_BECAUSE_NOT_FOUND vs
> SUBSTITUTE_BECAUSE_BEST/PENDING.

Dividing by 2 a dozen times or so times seems cheap compared to saving an array the whole time, no?

Comment 38

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #37)
> (In reply to Andrew Osmond [:aosmond] from comment #36)
> Can you include comments to describe this? Also the name could be clearer.
> mSuggestedSize?
> 

Agreed.

> > (In reply to Timothy Nikkel (:tnikkel) from comment #34)
> Wasn't there a bug recently where exactly this was happening? I'm fairly
> sure I remember such.
> 

With SVGs specifically? There were a number of bugs with similar problems (see comment 28) which I tested this patch set against to ensure I fixed them all.

> > (In reply to Timothy Nikkel (:tnikkel) from comment #35)
> Dividing by 2 a dozen times or so times seems cheap compared to saving an
> array the whole time, no?

I guess so. I will need to iterate and compare, so it is at most 31 + native sizes... I can easily fit that on an array on the stack. We really only need to iterate *once* (that isn't true right now, but I will correct that flaw now that I remember it) and not even through the whole thing if we compare native sizes first followed by decreasing factors of the largest size...

Comment 39

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #38)
> With SVGs specifically? There were a number of bugs with similar problems
> (see comment 28) which I tested this patch set against to ensure I fixed
> them all.

Yeah, svg specifically.

Comment 40

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #39)
> (In reply to Andrew Osmond [:aosmond] from comment #38)
> > With SVGs specifically? There were a number of bugs with similar problems
> > (see comment 28) which I tested this patch set against to ensure I fixed
> > them all.
> 
> Yeah, svg specifically.

Okay. I think SVGs would need a different approach. They don't necessarily have a given "native" size bound to them, and even if they did, they can upscale easily. Depending on the image itself, they might also not request valid sizes where the width/height ratio is the same.

The CPU usage generating the raster images was a limiting factor in the listed bugs. If the SVG problem was merely memory consumption, we could discard the least recently used surfaces or something the next time a new one is inserted. If it is CPU, I think the only way around it would be some sort of dynamic/relative acceptable scaling (e.g. if it has the same width/height ratio, then we could force it to prefer a downscale as long as its area doesn't stray too far...).

I'd want to see the bug first though, and I think it could be considered a separate/followup bug to what we are solving here. With some rejigging I imagine it could reuse a lot of the same machinery.

Comment 41

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add preference to control "factor of 2" mode for ImageSurfaceCache., v3 [carries r=tnikkel]

Update based on review feedback.

Comment 42

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Give image::LookupResult an optional preferred size to decode at when the surface is not found., v2

Comment 43

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state., v3

Comment 44

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add ImageSurfaceCache factor of 2 mode size calculations., v3

Comment 45

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. ImageSurfaceCache::LookupBestMatch should enter factor of 2 mode on cache misses., v3

Comment 46

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f200b3060fe7ef2f35478cfa356612fc3b58ee41

Comment 47

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8898346 [details] [diff] [review]
Part 6. ImageSurfaceCache::LookupBestMatch should enter factor of 2 mode on cache misses., v3

>-  Pair<already_AddRefed<CachedSurface>, MatchType>
>+  Tuple<already_AddRefed<CachedSurface>, MatchType, IntSize>
>   LookupBestMatch(const SurfaceKey& aIdealKey)

Can you document the what the fields in the return tuple mean?

Comment 48

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8880164 [details] [diff] [review]
Part 8a. Add "explicit" flag to ISurfaceProvider state to indicate when the caller won't accept substitutes., v1

"Explicit" is very generic. What about "specific" or "no substitutes"?

>diff --git a/image/DecoderFlags.h b/image/DecoderFlags.h
>index c4a4df0e59fe..bc54d8f91039 100644
>--- a/image/DecoderFlags.h
>+++ b/image/DecoderFlags.h
>@@ -18,17 +18,18 @@ namespace image {
>  * instead either influence which surfaces are generated at all or the tune the
>  * decoder's behavior for a particular scenario.
>  */
> enum class DecoderFlags : uint8_t
> {
>   FIRST_FRAME_ONLY               = 1 << 0,
>   IS_REDECODE                    = 1 << 1,
>   IMAGE_IS_TRANSIENT             = 1 << 2,
>-  ASYNC_NOTIFY                   = 1 << 3
>+  ASYNC_NOTIFY                   = 1 << 3,
>+  IS_EXPLICIT                    = 1 << 4
> };

Document the new flag, whatever name is ends up with.

Comment 49

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8880165 [details] [diff] [review]
Part 8. Add ImageSurfaceCache::Prune to discard surfaces which are not needed in factor of 2 mode., v3


>   /**
>+   * Attempts to remove cache entries (including placeholders) associated with
>+   * the given image from the cache, assuming there is an equivalent entry that
>+   * it is able substitute that entry with.
>+   *
>+   * @param aImageKey  The image whose cache which should be pruned.
>+   */
>+  static void PruneImage(const ImageKey aImageKey);

You should mention this only applies to factor of 2 mode images.

Comment 50

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. ImageSurfaceCache::LookupBestMatch should enter factor of 2 mode on cache misses., v4 [carries r=tnikkel]

Add comments requested in review.

Comment 51

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8a. Add "substitutable" flag to ISurfaceProvider state to indicate when the caller won't accept substitutes., v2

Changed from Explicit to Substitutable (along with the inverted meaning).

Comment 52

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8b. Add ImageSurfaceCache::Prune to discard surfaces which are not needed in factor of 2 mode., v4 [carries r=tnikkel]

Update based on name changes in part 8a.

Comment 53

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 9. Add factor of 2 state to the SurfaceCache memory reports., v4 [carries r=tnikkel]

Update based on name changes in part 8a.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e5edfee0b40beace1ed5c38a282074aa86fba39

Comment 54

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8903239 [details] [diff] [review]
Part 8a. Add "substitutable" flag to ISurfaceProvider state to indicate when the caller won't accept substitutes., v2

Having the flag for the non-usual case of "not substituable" seems better, and its only one more letter.

Comment 55

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #54)
> Comment on attachment 8903239 [details] [diff] [review]
> Part 8a. Add "substitutable" flag to ISurfaceProvider state to indicate when
> the caller won't accept substitutes., v2
> 
> Having the flag for the non-usual case of "not substituable" seems better,
> and its only one more letter.

I renamed this to CANNOT_SUBSTITUTE.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83c49f7c9c99
Part 0. Add imgIContainer::GetNativeSizesLength to determine a ceiling on the maximum number of expected, unique surfaces. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e5d359e10e
Part 1. Add preference to control "factor of 2" mode for ImageSurfaceCache. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/87b213841ad3
Part 2. Give image::LookupResult an optional preferred size to decode at when the surface is not found. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0c916b3960
Part 3. Break out ImageSurfaceCache::CompareArea from LookupBestMatch for reuse. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc167260154
Part 4. Add ImageSurfaceCache::MaybeSetFactor2Mode and state. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/536e6a1d3b4f
Part 5. Add ImageSurfaceCache factor of 2 mode size calculations. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1a5ce9c333
Part 6. ImageSurfaceCache::LookupBestMatch should enter factor of 2 mode on cache misses. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/71f26daac945
Part 7. ImageSurfaceCache::Lookup should enter factor of 2 mode on cache misses. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/c856f5132643
Part 8a. Add "substitutable" flag to ISurfaceProvider state to indicate when the caller won't accept substitutes. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/85cdb40c2621
Part 8b. Add ImageSurfaceCache::Prune to discard surfaces which are not needed in factor of 2 mode. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd1c5cd9b01
Part 9. Add factor of 2 state to the SurfaceCache memory reports. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e273a191e75d
Part 10. Add SurfaceCache gtests for factor of 2 mode. r=tnikkel

Comment 57

[Andrew Osmond [:aosmond] (he/him)]

I did a partial backout of just part 10. The reasoning being is the only two checks failed in the whole gtest -- the functionality still appeared to work, but why it decided to decode an extra surface on Windows specifically is very puzzling. There is *no* platform specific code afaik that could be affected.

Comment 58

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrew Osmond [:aosmond] from comment #30)
> Created attachment 8896225 [details] [diff] [review]
> Part 0. Add imgIContainer::GetNativeSizesLength to determine a ceiling on
> the maximum number of expected, unique surfaces., v2
> 
> Recent try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c64046493c529697616654b92d1c77ce0816664b

In my try here, the same gtests pass on Windows (save Windows 7 debug which has an unrelated failure...).

Comment 59

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1930f2a188
Backed out changeset e273a191e75d for Windows gtest failures.

Comment 60

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/83c49f7c9c99
https://hg.mozilla.org/mozilla-central/rev/b9e5d359e10e
https://hg.mozilla.org/mozilla-central/rev/87b213841ad3
https://hg.mozilla.org/mozilla-central/rev/4f0c916b3960
https://hg.mozilla.org/mozilla-central/rev/fcc167260154
https://hg.mozilla.org/mozilla-central/rev/536e6a1d3b4f
https://hg.mozilla.org/mozilla-central/rev/9d1a5ce9c333
https://hg.mozilla.org/mozilla-central/rev/71f26daac945
https://hg.mozilla.org/mozilla-central/rev/c856f5132643
https://hg.mozilla.org/mozilla-central/rev/85cdb40c2621
https://hg.mozilla.org/mozilla-central/rev/5cd1c5cd9b01