ScrollListener to improve performance

[sockeqwe]

Hi,
as promised I have spent some hours to work on a solution for the scroll performance issue. The result looks promising.

Please note that I would like to discuss my solution and a some critical points with you before merging this pull request.

So what I have did:

1. I have created a class called DispatchingQueueand have integrated it into the already existing Dispatcherclass. I have added two methods to Dispatcher:
   • void interruptDispatching()
   • void continueDispatching()
     So when Dispatcher.dispatchComplete(BitmapHunter) will be called the dispatcher will check if the dispatching has been interrupted before (by calling interruptDispatching(). If no, the "complete event" will dispatched directly. If the dispatching has been interrupted, then dispatching the "complete event" will be added / enqueued to the DispatchingQueue (internally stored in a class called DispatchJob). So after continuing dispatching (by calling Dispatcher.continueDispatching() ) all waiting DispatchJobs will be executed. The same workflow will run for Dispatcher.dispatchFailed(BitmapHunter hunter).
     QUESTION 1: Is it correct to do this only for this two methods or should Dispatcher.dispatchRetry() etc. run also with DispatchingQueue? I guess this two methods are enough right?
     The big advantage by doing this in the Dispatcher is, that the BitmapHunter can do his work asynchronously and only the fading will be interrupted . Also setting placeholder or the image from memory cache works immediately.
2. Calling Dispatcher.performCancel(Action) while dispatching is interrupted (which means that its likely that there are DispatchJobs waiting in the DispatchingQueue) will also dequeue any waiting DispatchJob associated with this Action (to be more precise: with the associated BitmapHunter).
   QUESTION 2: Is there another way a Action / BitmapHunter could be canceled that I have missed?
   The DispatchingQueue will be cleaned up (clearing the queue) after Dispatcher.shutdown()has been called.
3. I have written Unit Test for that (DispatchingQueueTest)
4. I have added two methods to Picasso class. So this two methods will be the public API for dispatching:
   • Picasso.interruptDispatching(): calls Dispatcher.interruptDispatching()
   • Picasso.continueDispatching(): calls call Dispatcher.continueDispatching()
5. I have added two default OnScrollListener implementations which can be found in the package com.squareup.picasso.scrolling
   • PicassoScrollListener: interrupts / continues if and only if the scroll state is IDLE. So fling or simply scrolling will cause the dispatcher to be interrupted. I think that the most users who have gerneral performance issues while scrolling will also have performance issues while scrolling a few pixels (not flinging). Therefore I have decided that this one should be "the default" behaviour
   • PicassoFlingScrollListener: like the name alredy suggests this OnScrollListener will interrupt while flinging (so normal scrolling will not interrupt dispatching).
6. I have added to the samples two Actities just to demonstrate how to work with this two listeners. (well its more a ugly copy & past of already existing samples; I was a little bit lazy and didn't think about architecture, inheritance etc.)

Last but not least there is one issue:
As you see in the code DispatchingQueue.interruptDispatching() will simply increment a counter and DispatchingQueue.continueDispatching()will decrement the counter. Dispatching is enabled if t counter == 0 I did that because I think there could be more than one ListViews displayed at the same time (like in a master / detail view on a tablet, or viewpager , etc. ). So far so good, but there is one critical issue with this counter solution (you can reproduce it also in the samples: for instance GridView with Scrolling). If dispatching has interrupted, for example by starting scrolling (the counter will be incremented to 1) and let the finger on the screen and rotate your device to landscape the PicassoScrollLister will not receive the IDLE Scroll State and the counter will not be decremented --> No further dispatching ...

QUESTION 3: Do you have any smart solution for this problem? My solution would be to use something like a WeakHashSet and changing interrupt / continue methods to something like this Dispatcher.interruptDispatching( Object interruptRequestor ) and Dispatcher.continueDispatching( Object interruptRequestor ). The interruptRequest could be the PicassoScrollListener. So the idea is to add the interruptRequest to the WeakHashSet and if the weakHashSet is empty that dispatching is enabled. So this should fix the problem described above. However there is no "WeakHashSet structure" in java. We could use WeakHashMap<Object, Void> instead. However I can't see a way how to get notified if an entry has been removed from WeakHashMap, because after removing an entry we have to check if dispatching should be continued or not.
Any suggestion or even smarter solution is welcome.

The sample apk file can be found here and the picasso library jar here

[sockeqwe]

I forgot to ask:
How to I apply/setup IntelliJ to use your check style @JakeWharton ? I didn't found a solution on google. Shame on me. Therefore Travis CI can not build.

Btw. the code of WeakHashMap can be found here .
There is a method private void expungeStaleEntries() where the referencequeue is polled. I guess that could be the right place to notify DispatchingQueue. Unfortunately this method is private, so we can't override. We could copy the code ... however it seems a ugly solution

[sockeqwe]

Im sorry, my fault, found squares code formatting for Intellij https://github.com/square/java-code-styles

[sockeqwe]

Regarding Question 3:
I have replaced the counter of the dispatching queue with a simple boolean. It works. In theory the following scenario would be possible:
If you have two ListViews in one layout and scroll / fling both at the same time, then the first ListView could interrupt dispatching while the second one asks to continue to dispatching. Concrete example:

1. ListView1 starts flinging which calls interruptDispatching()
2. While ListView1 is still flinging the user starts to scroll on ListView2 (which would also trigger interruptDispatching()) and keeps the finger on ListView2 while ListView1 stops flinging.
3. So ListView1 stops flinging which triggers continueDispatching().
4. Therefore Picasso continues to dispatching images also on ListView2, even if ListView2 is still in scrolling mode (finger / TouchEvent is still on ListView2).

The same problem could appear if you fling both ListViews with different speed which means that first ListView who stops flinging will trigger continueDispatching() and will cause to dispatching images also on the other ListView which is still flinging.

However, in a real world application the described scenario is unlikely to happen. And if it will happen it will only have impact on this scrolling gesture. So beginning a new scrolling gesture on any ListView would not cause problems. Furthermore it will work like expected with interrupting and continuing dispatching afterwards.

To solve this problem something like a counter should be implemented. I guess using ViewTreeObserver.OnWindowAttachListener could solve this problem. Unfortunately OnWindowAttachListener is available since Android API level 18. And I think writing a own PicassoListView which overrides View.onDetachedFromWindow() is not the right direction this library should develop.

[JakeWharton]

To solve question 3: require a tag when creating the listener and only start and stop requests with that tag.

[dnkoutso]

I will be reviewing this, this week.

[sockeqwe]

Hi, thanks.

Im not sure if I understand @JakeWharton correctly, but from my point of view adding a Tag (are we talking about an String?) for each request would be a clean solution, however it feels like an overkill to me to do so for each request.

Summary of what I have implemented so far:

1. Dispatcher can now be interrupted and resumed. So fading / setting image resource will be executed while not scrolling. But it does not improve the scroll performance very much.
2. PicassoExecutorService can now be paused and resumed. That's the game changes, because the scrolling performance issue is mainly caused by GC. So pausing the executor avouids new bitmap allocations and therefore GC will likely not run while scrolling.
3. Every interruptDispatching() or continueDispatching() call will interrupt or continue. So I don't use a counter like described in my first comment. In practice this works very well. We didn't face a problem or got problems reported by one of our 5.000 beta testers. You could download our APK (beta) https://db.tt/jjB2YazD which uses exactly the code of this pull request (don't blame me for the splash screen ).

[dnkoutso]

Nit: padding here.

[sockeqwe]

thanks for reviewing. I have pushed the desired changes.

[sarafdar]

RecyclerView is not accepting this ScrollListner!!

[sockeqwe]

Of course not! AbsListView.OnScrollListener and RecyclerView.OnScrollListener are different classes. However it's easy to implement your own PicassoScrollListener for RecyclerView. You simply have to call picasso.continueDispatching(); and picasso.interruptDispatching(). Some guys on StackOverflow and here on github have made a simple workaround: https://gist.github.com/slightfoot/fe35ce1fc3fde7daa0d6

They wrap the already existing one PicassoScrollListener into a RecyclerView.OnScrollListener ... However, you could simply copy the code of PicassoScrollListener and make it extend RecyclerView.OnSrollListener().

Btw. Im not sure if this pull request will ever be merged. So be careful if you have plans to use it in real apps in play store because there is no guaranty that this Pull request will be merged into Picasso 2.4 release.

@dnkoutso @JakeWharton Any status updates here? Are you working (or have plans to ) on your own solution?

[MarsVard]

so I guess it didn't get merged? too bad...

[lucasr]

FWIW, I really like @JakeWharton's 'tag' suggestion. Something like:

• Tagged requests

Picasso.with(context).
       .load(imageUrl)
       .into(imageView)
       .withTag("mylist");

• Pausing/resuming requests with a certain tag:

Picasso.with(context).pauseRequests("mylist");
Picasso.with(context).resumeRequests("mylist");

[sockeqwe]

I agree. I will try to find time to implement this next week. However, in many of my apps I need this scrolllistener for every screen (I use a lot of viewpager/fragments that displays ListViews). So setting a tag for each request seems a little bit to much boilerplatte code. Any suggestion how to deal with that or do you think that that's not a common use case? From my point of view it's the default use case: If you need this scrolllistener feature to reduce GC then its likely that you need this in your whole app.

[lucasr]

@sockeqwe The scroll listener's code would be so simple using these APIs that I wonder if it's worth having it as part of Picasso at all. But that's up to the maintainers to decide :-)

FWIW, I'm considering generalizing Smoothie's core functionality (https://github.com/lucasr/smoothie) to integrate with libraries like Picasso. Smoothie does some slightly fancier gesture tracking to figure out when it makes sense to trigger async loading in ListViews/GridViews.

[JakeWharton]

@lucasr Yep. I think the tag infrastructure is much more important than an explicit scroll listener. And then we can also have a cancel method for a whole tag which is also likely more important than pause/resume.

[sockeqwe]

@lucasr you are right, maybe my question was unclear. I thought it would be useful to make every request have a "default tag". So in my apps where I need the scroll listener everywhere, I do not want to specify a tag for every request. Instead every request will have a default tag. If you set a tag then the default tag will be overidden and the specified tag will be used. so that you can cancel/resume/pause all of them with the default tag (useful in my apps)

Picasso.with(context).into(imgageView) would be the same as Picasso.with(context).into(imageView).withTag(DEFAULT_TAG)

What do you think of a default tag?
Do you think it would be usefull to define multiple tags per requests?

[lucasr]

@sockeqwe Ah, ok, now I see what you mean. This could be accomplished by simply:

1. Not tagging any request.
2. Have an API like:

Picasso.with(context).pauseAllRequests();
Picasso.with(context).resumeAllRequests();

Having an pauseAll/resumeAll API seems safer/cleaner than exposing a public constant for the default tag. But maybe that's just me.

[lucasr]

@JakeWharton Yeah, a cancelRequests(String) would be useful too. I guess the relevance pause/resume depends on their semantics. Roughly speaking (as I'm not very familiar with how requests are managed in Picasso), the way I see them being used is something like:

1. You implement, say, an OnScrollListener that calls pauseRequests(tag) on FLING/TOUCH_SCROLL and resumeRequests(tag) on IDLE.
2. As you drag or fling the ListView with this custom OnScrollListener, the adapter will make several calls like Picasso.with(context).load(url).into(someImageView). These requests will just be queued and/or "re-targeted" as views get recycled.
3. Once the OnScrollListener enters IDLE state, the resumeRequests(tag) will resume all queued requests with the given tag on the last used target e.g. the ImageViews currently visible in the ListView.

pauseRequests(tag) could implicitly cancel ongoing requests with the given tag or maybe it could take a separate argument (a bit flag or a boolean) to explicitly set that.

[lucasr]

@sockeqwe Do you mind if I take over this issue? I think I have a pretty good idea of what needs to be done here. I already have a work-in-progress patch anyway.

[sockeqwe]

@lucasr No, you are welcome. I have started by implementing my own Queue for the ThreadPoolExecutor that internally map a tag to a list of requests and poll() returns a not paused Request. For canceling I have planed to do the same, but I didn't found time yet to implement it completely... I have only one wish: an API to pause / resume all request would be awesome. I mean pausing/canceling by tag is a pretty cool feature. However, in the most of my apps I need to pause/resume all requests and it would be great if picasso could pause/resume them without setting the same tag for each request by hand ... Let me know if I could help you somehow ...

[lucasr]

FYI: I've just created #665 with my suggested changes.

[lucasr]

@sockeqwe As far as I understand, this change shouldn't really be done on the executor/queue level. There is not necessarily a 1-to-1 mapping between a BitmapHunter and a request group (the name I'm using in my proposed API). For instance, a BitmapHunter might target actions from multiple groups at the same time. This means that what we want to pause/resume is the Action, not the BitmapHunter (which is what actually gets submitted to the executor). Have a look at #665 for more context.

As for your request to not requite a group tag in order to pause/resume requests, I ended up going with a default group marker defined as Picasso.DEFAULT_GROUP. You don't have to explicitly set it when creating new requests but you have to use it in the new pauseRequestGroup(String)/resumeRequestGroup(String) APIs.

[sockeqwe]

@lucasr You are right, I had an error in reasoning. Your code looks good. Well done!
I like the idea of having a Picasso.DEFAULT_GROUP.

[informramiz]

@lucasr from where I can get jar file for this fix? I really need it ASAP. Thanks

[JakeWharton]

Build a jar from master

On Wed, Jun 18, 2014 at 6:07 AM, sockeqwe notifications@github.com wrote:

Hi,
as promised I have spent some hours to work on a solution for the scroll
performance issue. The result looks promising.

Please note that I would like to discuss my solution and a some critical
points with you before merging this pull request.

So what I have did:

I have created a class called DispatchingQueueand have integrated it
into the already existing Dispatcherclass. I have added two methods to
Dispatcher:
- void interruptDispatching()
- void continueDispatching() So when
Dispatcher.dispatchComplete(BitmapHunter) will be called the
dispatcher will check if the dispatching has been interrupted before (by
calling interruptDispatching(). If no, the "complete event" will
dispatched directly. If the dispatching has been interrupted, then
dispatching the "complete event" will be added / enqueued to the
DispatchingQueue (internally stored in a class called DispatchJob).
So after continuing dispatching (by calling
Dispatcher.continueDispatching() ) all waiting DispatchJobs will be
executed. The same workflow will run for Dispatcher.dispatchFailed(BitmapHunter
hunter). QUESTION 1: Is it correct to do this only for this two
methods or should Dispatcher.dispatchRetry() etc. run also with
DispatchingQueue? I guess this two methods are enough right? The big
advantage by doing this in the Dispatcher is, that the BitmapHunter
can do his work asynchronously and only the fading will be interrupted .
Also setting placeholder or the image from memory cache works immediately.
2.

Calling Dispatcher.performCancel(Action) while dispatching is
interrupted (which means that its likely that there are DispatchJobs
waiting in the DispatchingQueue) will also dequeue any waiting
DispatchJob associated with this Action (to be more precise: with the
associated BitmapHunter).
QUESTION 2: Is there another way a Action / BitmapHunter could be
canceled that I have missed?
The DispatchingQueue will be cleaned up (clearing the queue) after
Dispatcher.shutdown()has been called.
3.

I have written Unit Test for that (DispatchingQueueTest)
4.

I have added two methods to Picasso class. So this two methods will be
the public API for dispatching:
- Picasso.interruptDispatching(): calls
Dispatcher.interruptDispatching()
- Picasso.continueDispatching(): calls call
Dispatcher.continueDispatching()
5.

I have added two default OnScrollListener implementations which can be
found in the package com.squareup.picasso.scrolling
- PicassoScrollListener: interrupts / continues if and only if the
scroll state is IDLE. So fling or simply scrolling will cause the
dispatcher to be interrupted. I think that the most users who have gerneral
performance issues while scrolling will also have performance issues while
scrolling a few pixels (not flinging). Therefore I have decided that this
one should be "the default" behaviour
- PicassoFlingScrollListener: like the name alredy suggests this
OnScrollListener will interrupt while flinging (so normal scrolling will
not interrupt dispatching).
6.

I have added to the samples two Actities just to demonstrate how to
work with this two listeners. (well its more a ugly copy & past of already
existing samples; I was a little bit lazy and didn't think about
architecture, inheritance etc.)

Last but not least there is one issue:
As you see in the code DispatchingQueue.interruptDispatching() will
simply increment a counter and DispatchingQueue.continueDispatching()will
decrement the counter. Dispatching is enabled if t counter == 0 I did
that because I think there could be more than one ListViews displayed at
the same time (like in a master / detail view on a tablet, or viewpager ,
etc. ). So far so good, but there is one critical issue with this
counter solution (you can reproduce it also in the samples: for instance
GridView with Scrolling). If dispatching has interrupted, for example by
starting scrolling (the counter will be incremented to 1) and let the
finger on the screen and rotate your device to landscape the
PicassoScrollLister will not receive the IDLE Scroll State and the counter
will not be decremented --> No further dispatching ...

QUESTION 3: Do you have any smart solution for this problem? My
solution would be to use something like a WeakHashSet and changing
interrupt / continue methods to something like this Dispatcher.interruptDispatching(
Object interruptRequestor ) and Dispatcher.continueDispatching( Object
interruptRequestor ). The interruptRequest could be the
PicassoScrollListener. So the idea is to add the interruptRequest to the
WeakHashSet and if the weakHashSet is empty that dispatching is enabled. So
this should fix the problem described above. However there is no
"WeakHashSet structure" in java. We could use WeakHashMap instead. However
I can't see a way how to get notified if an entry has been removed from
WeakHashMap, because after removing an entry we have to check if
dispatching should be continued or not.
Any suggestion or even smarter solution is welcome.

The sample apk file can be found here https://db.tt/VUr5EKkM and the

picasso library jar here https://db.tt/3l01sQXJ

You can merge this Pull Request by running

git pull https://github.com/sockeqwe/picasso master

Or view, comment on, or merge it at:

#561
Commit Summary

• DispatchingQueue
• PicasooFlingScrollListner; First UnitTests
• More unit testing
• Samples with Scroll Listener

File Changes

• M picasso-sample/AndroidManifest.xml
  https://github.com/square/picasso/pull/561/files#diff-0 (3)
• A picasso-sample/res/layout/sample_gridview_activity_scrolling.xml
  https://github.com/square/picasso/pull/561/files#diff-1 (27)
• A picasso-sample/res/layout/sample_list_detail_list_scrolling.xml
  https://github.com/square/picasso/pull/561/files#diff-2 (24)
• M
  picasso-sample/src/main/java/com/example/picasso/PicassoSampleAdapter.java
  https://github.com/square/picasso/pull/561/files#diff-3 (2)
• M
  picasso-sample/src/main/java/com/example/picasso/SampleGridViewActivity.java
  https://github.com/square/picasso/pull/561/files#diff-4 (2)
• M
  picasso-sample/src/main/java/com/example/picasso/SampleGridViewAdapter.java
  https://github.com/square/picasso/pull/561/files#diff-5 (27)
• A
  picasso-sample/src/main/java/com/example/picasso/SampleGridViewScrollingActivity.java
  https://github.com/square/picasso/pull/561/files#diff-6 (43)
• M
  picasso-sample/src/main/java/com/example/picasso/SampleListDetailActivity.java
  https://github.com/square/picasso/pull/561/files#diff-7 (5)
• M
  picasso-sample/src/main/java/com/example/picasso/SampleListDetailAdapter.java
  https://github.com/square/picasso/pull/561/files#diff-8 (21)
• A
  picasso-sample/src/main/java/com/example/picasso/SampleListDetailScrollingActivity.java
  https://github.com/square/picasso/pull/561/files#diff-9 (118)
• M picasso/src/main/java/com/squareup/picasso/Dispatcher.java
  https://github.com/square/picasso/pull/561/files#diff-10 (18)
• A picasso/src/main/java/com/squareup/picasso/DispatchingQueue.java
  https://github.com/square/picasso/pull/561/files#diff-11 (187)
• M picasso/src/main/java/com/squareup/picasso/Picasso.java
  https://github.com/square/picasso/pull/561/files#diff-12 (22)
• A
  picasso/src/main/java/com/squareup/picasso/scrolling/PicassoFlingScrollListener.java
  https://github.com/square/picasso/pull/561/files#diff-13 (64)
• A
  picasso/src/main/java/com/squareup/picasso/scrolling/PicassoScrollListener.java
  https://github.com/square/picasso/pull/561/files#diff-14 (74)
• M picasso/src/test/java/com/squareup/picasso/DispatcherTest.java
  https://github.com/square/picasso/pull/561/files#diff-15 (10)
• A
  picasso/src/test/java/com/squareup/picasso/DispatchingQueueTest.java
  https://github.com/square/picasso/pull/561/files#diff-16 (242)

Patch Links:

• https://github.com/square/picasso/pull/561.patch
• https://github.com/square/picasso/pull/561.diff

—
Reply to this email directly or view it on GitHub
#561.