915347 - Favicons should find the dominant color in a background thread

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Sriram Ramasubramanian [:sriram]]

Currently, getFaviconColor() runs on the UI thread. This can take anywhere between 10-27ms. Given that a frame has to be rendered within 16ms, spending this much time on this (on Galaxy Nexus) doesn't feel right. We should probably post it to a background task, update the favicon when the color comes back in a callback.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

I feel this approach is better. I could see it from the profiling bars. If this approach is fine, I can find other ways to make it better.

Comment 2

[:Margaret Leibovic]

Comment on attachment 803328 [details] [diff] [review]
Patch

I want to get ckitching's opinion on this. I know you've been talking on IRC, but have you seen his work in bug 914296?

Depending on how severe the problems you're seeing are, we may want to wait for bug 914296 to land before moving forward with this.

Does this perform worse than it did before the fig merge? If it's not a regression, I'd say we should wait for bug 914296.

Comment 3

[Sriram Ramasubramanian [:sriram]]

This is not a regression, it has been like this so far. I'm trying to make it better.

Comment 4

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> This is not a regression, it has been like this so far. I'm trying to make
> it better.

Okay, I'm going to remove the dependency on the new-about-home bug then.

I think we should wait to see what happens with Chris's new favicon cache, since I think that can lead to a cleaner solution.

I don't think there's a big rush here. If this turns out to be a good perf improvement, it would probably be low risk enough to uplift to aurora if we miss the merge.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #4)

> I don't think there's a big rush here. If this turns out to be a good perf
> improvement, it would probably be low risk enough to uplift to aurora if we
> miss the merge.

Agreed

Comment 6

[Chris Kitching [:ckitching]]

Comment on attachment 803328 [details] [diff] [review]
Patch

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

In summary:

It works. It has the desired effect - it does, indeed, make the dominant colour be calculated in the background thread which has a niceish performance improvement.

It's also horrible. It violates OOP, it kludges extra functionality into a class that's got a specific purpose - this is the way code ends up in a state of needing 80kb's worth of refactoring before it can be replaced. :P

Still, there's a deadline. This will do (Probably worth fixing the thing about not loading a background task if you don't nee to), but should very much be considered a short-term hack to make things suck less in 26, and not a long term solution.

::: mobile/android/base/Favicons.java
@@ +228,5 @@
>          }
>          return image;
>      }
>  
> +    public int getCachedFaviconColor(String key) {

You now have two adjacent functions with almost exactly identical bodies.

::: mobile/android/base/widget/FaviconView.java
@@ +76,5 @@
> +        setBackgroundDrawable(mBackgroundDrawable);
> +    }
> +
> +    private class LoadFaviconTask extends UiAsyncTask<Void, Void, Integer> {
> +        private final Bitmap mBitmap;

This probably shouldn't live in FaviconView - this is a UI component for rendering favicons - it probably shouldn't have something so backendey in it.
Additionally, with this and the patches from Bug 905685 we now have three classes called LoadFaviconTask, each of which does a subtley different thing. This is horrible OOP.

@@ +87,5 @@
> +        }
> +
> +        @Override
> +        public Integer doInBackground(Void... params) {
> +            return Favicons.getInstance().getFaviconColor(mBitmap, mKey);

Since doing a new task on the background thread is, itself, expensive, there is no point doing so if the colour we want is already in the cache.
A more sensible policy would be to only launch the background task if the colour cache misses. That way, you wouldn't need to pay the not insignificant overhead costs of doing the whole background-thread business.

Comment 7

[Sriram Ramasubramanian [:sriram]]

(In reply to Chris Kitching [:ckitching] from comment #6)
> Comment on attachment 803328 [details] [diff] [review]
> Patch
> 
> Review of attachment 803328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In summary:
> 
> ::: mobile/android/base/widget/FaviconView.java
> @@ +76,5 @@
> > +        setBackgroundDrawable(mBackgroundDrawable);
> > +    }
> > +
> > +    private class LoadFaviconTask extends UiAsyncTask<Void, Void, Integer> {
> > +        private final Bitmap mBitmap;
> 
> This probably shouldn't live in FaviconView - this is a UI component for
> rendering favicons - it probably shouldn't have something so backendey in it.
> Additionally, with this and the patches from Bug 905685 we now have three
> classes called LoadFaviconTask, each of which does a subtley different
> thing. This is horrible OOP.

That's the whole reason for having AsyncTasks. Instead of creating threads, the UI bits can do really small (but really big to be on UI thread) tasks in the background, and then update its UI.

> 
> @@ +87,5 @@
> > +        }
> > +
> > +        @Override
> > +        public Integer doInBackground(Void... params) {
> > +            return Favicons.getInstance().getFaviconColor(mBitmap, mKey);
> 
> Since doing a new task on the background thread is, itself, expensive, there
> is no point doing so if the colour we want is already in the cache.
> A more sensible policy would be to only launch the background task if the
> colour cache misses. That way, you wouldn't need to pay the not
> insignificant overhead costs of doing the whole background-thread business.

This is taken care of.

+            int color = Favicons.getInstance().getCachedFaviconColor(key);
+            if (color == -1) {
+                mLoadFaviconTask = new LoadFaviconTask(bitmap, key);
+                mLoadFaviconTask.execute();
+            } else {
+                updateImage(bitmap, color);
+            }

Comment 8

[:Margaret Leibovic]

Comment on attachment 803328 [details] [diff] [review]
Patch

Since this isn't a regression, I'm leaning towards just having us wait for Chris's changes to land, rather than landing this in the interim.

However, since we're going to be featuring the new about:home rewrite in 26, it would be really nice to have it perform as well as possible.

If we do go ahead with this patch, let's add some comments with bug numbers, indicating that this is something we intend to clean up in the near future.

Comment 9

[Sriram Ramasubramanian [:sriram]]

I was a bit equivocal on landing this. But then when I try to scroll history (with some 30-40 rows and some having favicons), I see it stutter a bit during the first time. The two main reasons:
1. Calculating dominant color in UI thread -- this bug.
2. The list changes the favicons even while scrolling -- which will cause a re-layout.

I guess it's good to have a patch like this, to help it scroll better in 26.

Comment 10

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.