isElementVisible performance

[priandsf]

The rows we are displaying are complex and thus the digest cycles are pretty expensive. By profiling some of the code, we figured out that isElementVisible triggers many browser style recalculations (forced reflow), see the attached picture. When I changed the isElementVisible like bellow:

      function isElementVisible(wrapper) {
         return true && wrapper.element[0].offsetParent;
         //return wrapper.element.height() && wrapper.element[0].offsetParent;
       }

then it suddenly became more efficient. From what I understand, this is for the rows that have an initial height of 0, but in our case all the rows always have the same fixed height, and are never hidden.
I'll be happy to submit a pull request with a parameter that prevents this check, but I would like to know what form it can have. For example, we can have a rowheight attribute that sets a fixed value for the row height. If so, and if we use this attribute during calculations, can this be used to reduce the manual calls to $digest, like in processUpdates or resizeAndScrollHandler? In the later, I'm not sure what a $digest needs to be triggered, particularly if no rows have been inserted/deleted. But I might be missing something :-)

[dhilt]

@priandsf First of all, thank you for the issue and your investigation! I knew that someday our visibility-watcher could play a trick on the ui-scroll performance. Unfortunately, your suggestion (remove element.height() check) doesn't work, it even breaks one of our 2 tests on visibility-watcher.

In general, I agree with you, that the fastest solution is to turn the entire feature off by some new external option (say, allowVisibilityWatch) and replace following lines of insertWrapperContent method

          if (!isElementVisible(wrapper)) {
            wrapper.unregisterVisibilityWatcher = wrapper.scope.$watch(() => visibilityWatcher(wrapper));
          }

with

          if (allowVisibilityWatch && !isElementVisible(wrapper)) {
            wrapper.unregisterVisibilityWatcher = wrapper.scope.$watch(() => visibilityWatcher(wrapper));
          }

This condition will control all possible isElementVisible method executions. But I want to investigate your situation a bit more. You say that your items have non-zero height initially and constantly, so wrapper.scope.$watch(() => visibilityWatcher(wrapper)) should not be executed at all, because if (!isElementVisible(wrapper)) condition should always give false. It means that this condition is the only place where your app deals with wrapper.element.height() expression which we suspect we don't like. It is being called per each row initialization; once per row. Can you confirm this? This would mean that calling wrapper.element.height() once per new row initialization is expensive enough, and we really may to try to improve it with allowVisibilityWatch option.

PS digest is being forced only when visibility-watcher works, which should not occur in your case with constant non-zero heights.

[priandsf]

Thanks for you reply Denis. And you're correct, the culprit is wrapper.element.height(), called from insertWrapperContent(), which triggers the reflow. In our case, visibility watchers are never created as the elements are always visible.

I've been playing with the library a bit, trying to understand its internals. It takes some time to dive within its intimate implementation, so I feel like a sorcerer's apprentice. In particular, I'm not sure about all the use cases it supports. Nevertheless, I experimented with a new fixed rowheight attribute. It plays the role of your flag fore-mentioned, plus it avoids some height calculations. Actually, when this attribute is set, it

• Never calculates the real height of the elements, but used the fixed value
• Assumes that the elements are always visible
• Doesn't call $digest during resizeAndScrollHandler()

All of that helped a lot from a performance standpoint. The reflow don't happen anymore, and the scroll is way smoother because it doesn't trigger $digest during the scroll unless it needs to load rows. So far it seems to work with our screens.

I also commented out the $digest in processUpdates() when there is a fixed row height. It also helped from a performance standpoint and seems to work as well. Not sure if I've done too much here or not...

If you want to have a look, I pushed the code to this branch: https://github.com/priandsf/ui-scroll/tree/Added-static-row-height
I'll be more than happy to get your feedback/advises/ideas...

[dhilt]

@priandsf You are doing great job! I opened PR and we'll be able to continue discussion within the PR as it might be much more convenient. Also, may I ask you to add me to your fork's collaborator list as I want to push commits along with you?

[priandsf]

Sure - Just did it. I meant to have done that earlier... :-)