1048626 - Provide a minimal background thread pool for long-running tasks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Capella [:capella]]

Spinning off from bug 1016686 ...

The idea is to quickly segregate lengthy background tasks (favicons / network related, etc) from faster / more-predictable tasks including local DB access, without taking a bigger leap to full blown multi-thread pools.

Comment 1

[Richard Newman [:rnewman]]

(Worth phrasing in these terms, I think.)

Comment 2

[Mark Capella [:capella]]

So to be clear, this bug isn't my last patch for a newSingleThreadExecutor()?
https://bugzilla.mozilla.org/attachment.cgi?id=8466651 ...

But a newFixedThreadPool() (with phased in consumers), similar to the WIP patch I shared with ckitching but haven't posted anywhere yet?

Comment 3

[Richard Newman [:rnewman]]

A singleThreadedExecutor is a one-thread pool. That sounds minimal to me :)

Comment 4

[Mark Capella [:capella]]

Attachment: bug1048626.diff

Moving and expanding the previous WIP a little ...

In the original patch I decided to let LoadFaviconTask.closeHTTPClient(); complete on the normal background thread, with the thinking that it was just closing local connection / socket information, and wouldn't require a LongRunning thread.

However, with this version, I decided that there's no harm posting it to the LongRunning Thread instead, and has a possible additional benefit of following any other "LoadFaviconTask" workers through the Thread maintaining a logical arrival order.

Take some time and have a look ... I'm letting it bake locally :-)

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8467884 [details] [diff] [review]
bug1048626.diff

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

Don't forget to clean up the commit description.

::: mobile/android/base/favicons/LoadFaviconTask.java
@@ +73,5 @@
>      }
> +
> +    public LoadFaviconTask(String pageUrl, String faviconUrl, int flags, OnFaviconLoadedListener listener,
> +                           int targetWidth, boolean onlyFromLocal) {
> +        super(null);

Do we need to extend UiAsyncTask now?

@@ +296,5 @@
>  
>      @Override
> +    // LoadFavicon tasks performed on unique background executor thread to avoid network blocking.
> +    public final void execute(Void... unused) {
> +        getUiHandler().post(new Runnable() {

Can you explain why we post a runnable to the UI thread in order to post a runnable to the background thread?

@@ +565,5 @@
>          // This work must be done on a background thread because it shuts down
>          // the connection pool, which typically involves closing a connection --
> +        // which counts as network activity. This isn't a lengthy task, but we
> +        // queue up behind outstanding favicon tasks in the LongRunning Thread.
> +        if (ThreadUtils.isOnLongRunningThread()) {

We close the HTTP client bluntly, I think. Consider whether we want to do it on the background thread anyway, in order to force the favicon tasks to die, or whether doing so will cause them to reawaken it.

::: mobile/android/base/util/ThreadUtils.java
@@ +33,5 @@
> +    // LoadFaviconTasks are posted to a unique thread.
> +    private static final String GECKO_LONG_RUNNING_POOL_PREFIX = "GeckoBackgroundPool-";
> +    private static final String GECKO_LONG_RUNNING_THREAD_PREFIX = "-Thread-";
> +    private static final ExecutorService longRunningExecutor =
> +        Executors.newSingleThreadExecutor(new GeckoLongRunningThreadFactory());

I'm uneasy about this being in Utils. You're creating this thread pool in a static initializer as soon as anyone calls ThreadUtils.isOnUiThread() or similar. That's probably a bad idea -- unpredictable performance characteristics.

@@ +124,5 @@
>      }
>  
> +    public static void postToLongRunningThread(Runnable runnable) {
> +        longRunningExecutor.execute(runnable);
> +    }

I'm not convinced that this really gives us much additional value. I mean, an executor *is* a nice abstraction around running shit, no?

@@ +205,5 @@
>          return isOnThread(sBackgroundThread);
>      }
>  
> +    public static boolean isOnLongRunningThread() {
> +        return Thread.currentThread().getName().indexOf(GECKO_LONG_RUNNING_POOL_PREFIX) == 0;

.startsWith. But do we need this at all?

@@ +243,5 @@
>      }
> +
> +    /**
> +     * A simple Thread Factory to create Threads that can execute our Long-Running tasks.
> +     * Currently we're only called-on to generate one, for our SingleThreadExecutor. We

called on

@@ +274,5 @@
> +                thread.setDaemon(false);
> +            }
> +
> +            if (thread.getPriority() != Thread.NORM_PRIORITY) {
> +                thread.setPriority(Thread.NORM_PRIORITY);

Is there a reason not to just make these calls regardless?

Comment 6

[Mark Capella [:capella]]

Attachment: bug1048626.diff (v2)

Ok ... mmmm ... "minimal" is the key word then :)

I guess closing the client on the regular background thread doesn't present a problem. Any tasks outstanding should have been cancelled and/or completed and/or will bail before waking the network again if I now read it right.

Comment 7

[Mark Capella [:capella]]

fyi, quick testing shows us saving ~320ms (+-300) per network lookup on GeckoBackgroundThread.

And I pushed this patch to try, and everything passes.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8468745 [details] [diff] [review]
bug1048626.diff (v2)

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

This would be r+ with nits, but I want to double-check the canceling stuff.

::: mobile/android/base/favicons/Favicons.java
@@ +337,5 @@
>  
>              Log.v(LOGTAG, "Cancelling favicon load " + taskId + ".");
>  
>              LoadFaviconTask task = loadTasks.get(taskId);
> +            cancelled = task.cancel();

return task.cancel();

...

@@ +342,2 @@
>          }
>          return cancelled;

… and then clean this up?

(Disregard this comment if more lines of context makes it nonsensical.)

::: mobile/android/base/favicons/LoadFaviconTask.java
@@ +294,5 @@
>      }
>  
> +    // LoadFavicon tasks performed on unique background executor thread to avoid network blocking.
> +    public final void execute() {
> +        Favicons.longRunningExecutor.execute(new Runnable() {

Be prepared for this to throw RejectedExecutionException, if the pool has been shut down (which you can check with isShutdown()).

In that case you should call onCancelled on the UI thread.

@@ +322,5 @@
> +    public final boolean isCancelled() {
> +        return mCancelled;
> +    }
> +
> +    protected Bitmap doInBackground() {

I think this can now be /* inner-access */ instead of protected.

@@ +491,1 @@
>      protected void onPostExecute(Bitmap image) {

I think this can now be /* inner-access */ instead of protected.

@@ +535,1 @@
>      protected void onCancelled() {

I think this can now be /* inner-access */ instead of protected.

Comment 9

[Lucas Rocha (:lucasr)]

Some food for thought:

what I'd *really* like to see is a better separation between the Favicons API and its underlying threading framework i.e. the threading framework should wrap "around" the API instead of being part of it under the hood.

The Favicons API itself should be thread-safe (it already is) and simply have some assertNotOnUiThread() calls here and there. This would allow us to implement proper support for favicon loading in Picasso and avoid having to manually handle view recycling whenever we need to show favicons.

More specifically, I'd like us to move towards something as simple as:

ImageLoader.from(context)
           .loadFavicon(TARGET_URL)
           .into(imageView);

We could embed things like target favicon size into the target URL. Something like:

gecko.favicon://encoded_page_url/target_size

There are probably some trickier details that we'd need to sort out around memory caching but it's doable.

Splitting the API from the threading framework is a good thing to do independently of my Picasso plans though.

Comment 10

[Mark Capella [:capella]]

I'll personally be happy to keep that in mind. I'm not terribly tuned in to the Picasso project / implementation, though I see you view that as just one parallel effort to be aware of in regards to future favicon-related enhancements.

I have a few personal ideas that has me looking to help in this area of the code, and ckitching has suggested that I also look at Bug 914027 ... though that then led me to a series of inter-related bugzillas in the 40 or so backlog under the "Favicon Handling" component I'm aware of, but yet to untangle / mentally prioritize :-)

I find scoping information / "food for thought" to be appreciated.

Comment 11

[Mark Capella [:capella]]

Attachment: bug1048626.diff (v3)

I cleaned up the cancelFaviconLoad() method.

I left the three "possible" /* inner-access */ methods alone as I didn't see using javap that they were generating synthetic accessors. It looked like the |mCancelled| var I moved into the class /was/ however, so I changed it.

I added an isShutdown() check before the Favicons.longRunningExecutor execution, rather than relying on a try/catch.

Finally, I updated a couple of fooUrl var's to fooURL in code we're touching with this patch, for simple consistency.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8470367 [details] [diff] [review]
bug1048626.diff (v3)

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

Thanks for being so thorough!

::: mobile/android/base/favicons/Favicons.java
@@ +61,5 @@
>  
>      // The density-adjusted maximum Favicon dimensions.
>      public static int largestFaviconSize;
>  
> +    // Threadpool for long-running Favicon Tasks.

Nit: "Executor for long-running favicon tasks." or "Thread pool for …".

@@ +346,5 @@
>  
>      public static void close() {
>          Log.d(LOGTAG, "Closing Favicons database");
>  
> +        // Close the Thread to new tasks.

Nit: s/Thread/executor.

::: mobile/android/base/favicons/LoadFaviconTask.java
@@ +292,5 @@
>          // Having downloaded the image, decode it.
>          return FaviconDecoder.decodeFavicon(buffer, 0, bPointer + 1);
>      }
>  
> +    // LoadFavicon tasks performed on unique background executor thread to avoid network blocking.

LoadFaviconTasks are performed…

@@ +296,5 @@
> +    // LoadFavicon tasks performed on unique background executor thread to avoid network blocking.
> +    public final void execute() {
> +        // Ensure our executor is available.
> +        if (Favicons.longRunningExecutor.isShutdown()) {
> +            onCancelled();

Which thread is onCancelled documented to be called on?

You should either be asserting that we're on that thread now, or delivering a runnable. Line 309 suggests that this should be the UI thread.

@@ +300,5 @@
> +            onCancelled();
> +            return;
> +        }
> +
> +        Favicons.longRunningExecutor.execute(new Runnable() {

You still need to be prepared for this to throw.

The executor could be shut down between line 299 and line 304, in which case this call will be rejected.

@@ +497,1 @@
>      protected void onPostExecute(Bitmap image) {

I was suggesting that these *should be* inner-access; they won't be generating accessors because they're protected. Try making them private and see if they cause accessors to be generated!

Comment 13

[Mark Capella [:capella]]

Attachment: bug1048626.diff (v4)

:-) Gotta be getting close !

re: Which thread is onCancelled documented to be called on?
   Based on the code pattern, and testing/logging I did I expected it to be run on the UiThread as you did. (Didn't notice any documentation). But adding the assert protects against future mis-use.

re: You still need to be prepared for this to throw.
   Yes. It crossed my mind, but I punted instead of doing the further homework based on a flawed reading of your suggestion.

re: I was suggesting that these *should be* inner-access
   Making them private does indeed generate accessors. Use of either /* inner-access */ notation or "protected" modifier avoids that. Unless I'm still being obtuse, I believe now you were just preferring the /* inner-access */ notation.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8470412 [details] [diff] [review]
bug1048626.diff (v4)

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

::: mobile/android/base/favicons/LoadFaviconTask.java
@@ +324,5 @@
> +    }
> +
> +    public final boolean cancel() {
> +        mCancelled = true;
> +        return mCancelled;

Just return true.

Comment 15

[Mark Capella [:capella]]

(awesome)
https://tbpl.mozilla.org/?tree=Try&rev=803eb41538ad

Comment 16

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/9fdc9d534e24

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9fdc9d534e24

Comment 18

[Wesley Johnston (:wesj)]

This seems like something that would make a nice newsgroup post with some minimal example code? Either of you mind doing that?

Comment 19

[Mark Capella [:capella]]

Answering for myself, I'm always happy to explain interesting things I've learned while implementing new code. I'm not sure how much value I could pass along here ... (?) This is a simple prototypical use, and the title nails it re:"minimal" background thread :-) At the risk of being modest, I'm sure I can find links discussing this more eloquently than I.

The other hesitation I'd have is that this was a targeted solution that, while benefiting the general code base by speeding up through-put on the standard Threadutils' GeckoBackgroundThread, doesn't provide much of an opportunity for other consumers to tie into. (This is simply a new single / parallel thread for favicons.)

We had discussed and I had prototyped another very rough version of Threadpool-ing that actually involves providing multiple concurrent threads for background tasks when we were feeling frisky :)

That might be fun to talk about. Is this what you have in mind?

Comment 20

[Wesley Johnston (:wesj)]

Ahh. You may be right. The title to this bug confused me into thinking we'd implemented a simple opt-in thread pool, but it looks like we just created (basically) a separate Favicon thread that uses its own single-thread "pool" :(

I'll just keep on dreaming.

Comment 21

[Mark Capella [:capella]]

It can still be done! aiui, we just punted on the full blown pool newFixedThreadPool() executor, being unsure of the safest way to phase in it's adpotion across the app, avoiding interdependencies and all that.

I'll offer to help make your dreams a reality, but, you-know ... that sounds silly :)