Refactor the scroll to use an interval instead of firing every digest

[observstream]

This is a commit to fix #45 and is based on http://ejohn.org/blog/learning-from-twitter/. It uses the scroll event to set a variable that an interval, running every 100 milliseconds, watches for and when found fires the adjustBuffer function if it isn't already loading.

This is a large commit (albeit a not very complicated one). The changes I have made fall into three types.

1. Changing code (just src/ui-scroll.js)
2. Changing the angular version of all demo pages to be at least 1.2.0
3. Updating tests to take the interval into account

Breaking Change - The only change that could break current implementations is the requirement for Angular 1.2 or above.

Any feedback would be wonderful!

[mfeingold]

I am hesitant to sign off on the suggested implementation. Primarily because of elimination of $timeout in some use cases. The problem with this is that the impact of such removal is only felt on more complex templates. I doubt that the either existing tests or examples cover such use cases sufficiently.

The sneakiness of the problem is due to the fact that it only shows up if the height of the item template changes as a result of the template instantiation.

Besides I am trying to figure out if the scroll even flood can be reduced even further. One approach I am considering is to unsubscribe from all events after receiving the first one and only subscribe back after the pending queue becomes empty.

One more note - I think that resize event deserves the same treatment as the scroll

[aantipov]

I think resizeHandler should be renamed to scrollHandler, and resizeAndScrollHandler to resizeHandler

[observstream]

Agreed.

[mfeingold]

There is another issue I am trying to wrap my head around which may or may not be fixed by this. Here is the repro:

1. open this example
2. Click on the list in the bottom and scroll it up for a couple of dozen items.
3. Click on the list on the top and while it has focus press the arrow down key

AFAIK on all browsers except Chrome for Mac it works as expected. On Chrome for Mac the top list instead of slightly moving down scrolls way up. I am still trying to figure this one out

[aantipov]

The sneakiness of the problem is due to the fact that it only shows up if the height of the item template changes as a result of the template instantiation.

hmm, what is the role of $timeout in that case?
$timeout does 2 things - deferring the execution and calling $digest.
I don't see why any of them might be useful during scrolling.

[mfeingold]

$timeout would not help, but compounding scroll events might. I suspect the problem has to do with some race condition between multiple scroll events.

[aantipov]

@mfeingold So what are your apprehensions about current solution with $interval and removing $timeout?
I'm also thinking about a bit more sophisticated solution - turning $interval on at the beginning of scrolling and turning it off at the end of scrolling, and also about passing false as the 4th parameter to $interval to avoid $digest call.

[observstream]

@mfeingold @aantipov Thanks for the feedback. I will re-pull with the suggested changes.

[observstream]

@mfeingold For your issue with Chrome on Mac, I am not getting your exact problem. I am getting an issue where when I scroll with the keyboard, as soon as it loads the next set, the key event gets interrupted and it stops scrolling. I have to click on the viewport again to scroll some more.

[observstream]

@mfeingold I can understand your apprehension for leaving $timeout and moving to $interval. You are correct that this is affected by templates being loaded that change height. We are loading lists of images.

The current solution only uses timeout to enter the digest cycle. The code does wait to fire another timeout if the digest is currently running, but it is way more frequent than it needs to be.

@aantipov @mfeingold I am also wondering if we can do this outside of the digest cycle. @mfeingold I would love to know what problems you have run into by having the code outside of the digest cycle.

[observstream]

I have removed the $timeout from the resize and also updated the setIsLoading function to only use timeout when a isLoading parameter is set on the $attr. Also, I have updated the tests to remove $timeout. There are several conflicts with the current branch, but all of them are with recent updates to the tests and should be able to merge. I will go through and merge these issues and recommit.

[observstream]

I have uploaded a merge that syncs the current master to my fork. I am getting errors with the following two tests that were changed since the last time the fork was updated.

uiScroll datasource with 20 elements and buffer size 3 - constrained viewport should call get on the datasource 1 more time (6 total)

uiScroll datasource with 12 elements and buffer size 3 (fold/edge cases) [fold frame, scroll down] should call get on the datasource 1 extra time

Basically in both of these cases the number of times the adapter get function is being called the old number (it appears).

[mfeingold]

I am just thinking out loud here, so bear with me.

The root of this nifty gadget is the adjustBuffer function. It waits for the item templates to be processed (that's why the $timeout), then determines if anything is to be appended, an if not is anything is to be prepended. If it is, a fetch request is pushed in the queue and if the queue was empty the first fetch request is kicked off (see enqueueFetch).

The fetch calls the get method of the datasource and when the datasource reports back with the results it inserts received data into the buffer and closes the loop by calling AdjustBufferAfterFetch, which is really the same thing as the adjustBuffer with a few variations related to proper processing the use case when the viewport changes visibility.

Note that although in majority of cases the datasource returns the data asynchronously, the requests are processed in strictly sequential manner - a new one is initiated only after the processing of the previous one is completed.

Now, with scrolling (and, potentially resizing) this clear picture looses its clarity. Every scroll/resize event initiates its own chain of adjustBuffer calls causing this excessive (and redundant) activity. Ideally the best way to approach this would be to trigger the initial adjustBuffer (possibly with a short delay), check the necessity to load more data in the usual manner (after a new portion is received) and only resume watching the events after the disturbance caused by the scroll/resize have settled.

There are 2 problems with this though. Firstly - how do you know that the scrolling ended? and secondly I am not sure if scroller can handle properly the data load if scrolling took you too far from the location of what's in the buffer,

Now using the interval although can reduce the number of adjustBuffer chains does not really address the root cause of the problem. We will have to choose between sluggish response (interval too big) or too many adjustBuffer chains (interval too small)

[aantipov]

Well, guys, we need some time to think up and agree on an appropriate solution.
For now, as a first aid, may be we can just remove calling the digest cycle on each scroll event?
It can be easily achieved by passing false as a third parameter to the $timeout.
If we do can do it, that will significantly improve the overall performance while scrolling.
What are you thinking about it?

[observstream]

The only question I would have with removing the digest cycle completely is what that does to the loading of the item templates? Also, if someone wants to update the isLoading variable, the digest cycle would have to run (see my most recent changes where I changed the setIsLoading function to have the digest cycle)

I am OK with waiting on this change until we vet the different solutions.

[mfeingold]

I am not sure what do you mean 'remove the digest cycle'. Running
shouldLoadTop/Bottom over something other than fully instantiated item
templates does not work. What can be done is elimination of redundant
adjustbuffer calls

Michael Feingold, President
401 Huehl RD, Suite 2A,
Northbrook, IL 60062
Email: [email protected]
Web: hill30.com http://www.hill30.com

On Wed, Mar 9, 2016 at 9:15 AM, J Humes [email protected] wrote:

The only question I would have with removing the digest cycle completely
is what that does to the loading of the item templates? Also, if someone
wants to update the isLoading variable, the digest cycle would have to run
(see my most recent changes where I changed the setIsLoading function to
have the digest cycle)

I am OK with waiting on this change until we vet the different solutions.

—
Reply to this email directly or view it on GitHub
#56 (comment).

[observstream]

I have done more checks and tests on my own dataset and can see there is a large amount of code that is run way too often (adjustBuffer -> $timeout -> processBufferedItems + viewport.shouldLoadBottom() || viewport.shouldLoadTop()) All of these functions are run for every scroll event as long as there isn't a current digest cycle. Currently, this means ~100 times a second during scrolling. If any future commit adds any increase in time these functions require to run, or a user is on a less than optimal machine, it will degrade the performance of the scroll event.

I can totally see a problem with not using the digest cycle because of the needing the templates to load (although my most recent commit seems to have addressed almost all of these issues), so I don't think we need to change the system to remove the digest cycle. We do need to address the reality that we can call the adjustBuffer 10 times less and not really have any performance issues in the scroll. It also gives adjustBuffer at least a full 10 milliseconds to finish before the next iteration is called.

I am unable to see how it will be possible without some kind of interval. We could add a minimum timeout to the adjust buffer before it is run, but that is basically the same thing. I do think an investigation of destroying the interval when not scrolling is a possible improvement.

I was able to fix a lot of the problems I have with the adapter.reload function by forcing all the items in the scroll to have a fixed height. That way if an image is still needing to be loaded before the next adjust buffer, then elements don't resize.

[aantipov]

Running shouldLoadTop/Bottom over something other than fully instantiated item
templates does not work.

Ok, I understand that adjustBuffer is called not only when scrolling, but in a lot other cases where we do need $digest cycles.
What I was talking about is to remove unnecessary $digest calls when scrolling.
Actually, as I can see, @jahumes have implemented what I'm talking about - created adjustBufferWithoutTimeout function and call it when scrolling. So he got rid of digest calls during scrolling.

I realise that introducing $interval might seem doubted and need more thoughtful discussions, this is why I suggest to go now with little changes - just getting rid of $digest calls when scrolling - create adjustBufferWithoutTimeout function and call it on scroll event. This little change will give us huge performance gains right away.

[mfeingold]

I think that while trying to eliminate some $digest cycles might make sense in some use cases, there is another angle which might have better impact on performance. I am talking about elimination of redundant calls to adjustBuffer.

You see - any call to adjustBuffer done while there are outstanding fetch requests is redundant, because when the fetch is completed, the adjustBuffer will be called again.

Elimination of redundant adjustBuffer calls generated by scroll events is simple - just unsubscribe from the event. Finding the right moment to subscribe again is somewhat trickier, but this way all redundant adjustBuffer calls will be eliminated, and all remaining ones will be essential for proper functioning of scroller.

Once we have this one nailed, we can have a look at what can be done to speed up the adjustBuffer calls, might be removing $apply in some use cases.

[aantipov]

Finding the right moment to subscribe again is somewhat trickier

Hmm, why not to subscribe again once we completely processed the fetch and there is no need to load more data?

[mfeingold]

@jahumes @dhilt @aantipov Guys I just checked in what I hope is a draft of the fix for the performance problem. I checked it in a new branch scrollingOptimization. The best way to check it out is by using scopeDatasource example from the branch I added some counters there. There are 2 commits there first the one with the counters exposed, and then with what I hope is a fix.

Can you please check it out?

P.S. amazingly enough the fix seems to fix also the Chrome on Mac problem I reported before

[mfeingold]

fixed with different approach in #66