huge performance issues

[aantipov]

Hi
Currently my app experiences big performance issues when scrolling through the list of items.
Having done some investigation I found out that each scroll event causes 2 digest cycles!!!
Taking into account that each digest cycle runs a lot of watchers (~1000) we are getting an unresponsive app.
It's a very bad idea by itself to do anything on every scroll event, even John Resig long time ago warned against using it http://ejohn.org/blog/learning-from-twitter/
But I wonder why do we need to run digest cycles at all when scrolling?

BTW, I found the way to get rid at list of one redundant digest call.
Look at this function

resizeAndScrollHandler = function() {
  if (!$rootScope.$$phase && !adapter.isLoading) {
    adjustBuffer();
    return $scope.$apply();
  }
};

$scope.$apply(); call here is redundant because adjustBuffer() calls $timeout function, which in its turn calls $scope.$apply();. So we can get rid of $scope.$apply() in the code above.

[mfeingold]

Submit a pull request. This call to $apply is a throwback from the time when in certain use cases there was no calls to timeout. Another thing I keep going back to is to disable scroll handler for as long as there are pending requests

[aantipov]

I'd like to submit a pull request, but the problem is that I haven't written in coffee quite a lot of time so I'm not sure about it.
I'm not yet quite into the logic of the code, so I just wanted to know it there any real reason for a use of digest calls while scrolling? Are there cases when we do need them?

[mfeingold]

After a batch of elemens is instantiated, the decision whether more elements are needed or scroller has enough to fill the viewport) is based on the dimensions and positions of the instantiated elements. And this information is only becomes available after the elements are fully instantiated.

[aantipov]

Agree. But still didn't get the idea why we need to constantly run digest cycles when scrolling

[mfeingold]

We do not. We just need to cover the entire span the scrolling went through. I think it makes sense to ignore scrolling events after the first one fired. The problem is when to re subscribe. I am playing with the idea of waiting for all pending requests to complete and re subscribe after that. It will not eliminate all redundant requests, but, I hope, will reduce the number of them. And this redundunat scope.$apply definitely should be removed, although I need to double check for edge cases

[nestak]

Hi,
would it be a good idea to fire this function only when scrolling stops for a while?
Something like:

var postponed = null;
resizeAndScrollHandler = function() {
  clearTimeout(postponed);
  postponed = setTimeout(proceed, 100);

  function proceed () {
    if (!$rootScope.$$phase && !adapter.isLoading) {
      adjustBuffer();
      return $scope.$apply();
    }
  }
};

[dhilt]

@aantipov I've double checked and commited your redundant digest call fix, thank you.

@nestak It's an interesting idea but there are some potentialy bad points I see. 1st is that this delta (100ms) affects the UI response. 2nd is that the size of this delta is empirical data -- if you make it too small then scroll events will be fired in-between user' scrollings -- and I guess it also may depends on browser' realization... anyway this idea requires additional research... thanks!

[aantipov]

@dhilt

I've double checked and commited your redundant digest call fix, thank you.

you commited it to dist/ui-scroll.js, not to src/ui-scroll.js :)
BTW, this change break the tests, so I'm not sure we can do it

[dhilt]

yeah, thanks for this note; i've recommited this fix; it includes also a simple test fix

[aantipov]

Cool!

[observstream]

I have committed a potential fix for the performance issues by attaching the check to an interval running every 100 milliseconds instead of on the scroll event (about every 6-10 milliseconds).

[aantipov]

As for me - the decision looks fine. I just added several notes to the PR. But we do need to test it thoroughly.
Also may be is it worth to change interval time to a smaller value, say 80ms?

Anyway, we need to hear a word from @mfeingold and @dhilt

[mfeingold]

fixed in #66