Fixed row height optimization #221

dhilt · 2019-06-04T20:53:21Z

Based on issue #220.

Visibility-watcher feature might be optional.
When a row height is fixed, the performance could be improved.

dhilt · 2019-06-05T15:58:38Z

demo/index.html

@@ -64,6 +64,11 @@ <h1 class="page-header">Scroller Examples</h1>
        Bottom visible (Adapter)
      </a>
    </li>
+    <li>
+      <a href="fixedRowHeight/fixedRowHeight.html">


I added demo, after npm start it is available as http://localhost:5005/fixedRowHeight/fixedRowHeight.html

dhilt · 2019-06-05T15:59:39Z

src/ui-scroll.js

+
+        // PHIL: Provide a fixed row height
+        // 
+        const rowHeight = parseNumericAttr($attr.rowHeight, null, false);


switched to camelcase, which means row-height="20" usage

dhilt · 2019-06-05T16:13:15Z

src/ui-scroll.js

@@ -386,8 +397,11 @@ angular.module('ui.scroll', [])
          // We need the item bindings to be processed before we can do adjustments
          !$scope.$$phase && !$rootScope.$$phase && $scope.$digest();


@priandsf This is the bottleneck of the new demo (http://localhost:5005/fixedRowHeight/fixedRowHeight.html). And this $digest can't be removed, it responses for manual data-binding and transforming a row template into final html with all the data and handlers. Running the demo, you'll see that removing this $digest breaks the demo app.

From the other hand, I didn't see any significant difference in the performance with and without rowHeight (if don't touch this $digest). Probably the demo is not good, and I would ask you to update it if you can. The demo must show a difference with and without rowHeight. But if this particular $digest is the only call that "can" improve situation, then I would say probably we are on the wrong way. I don't see a situation where this $digest can be removed.

So, please look at the demo, try to reproduce performance changing and let me know, what do you think. Thanks!

So you're right, commenting $digest in processUpdates() was too aggressive. It worked in my case as we had another trigger for $digest. I suspect this was not good :-)
I split the behavior into row-height and allow-visibility-watch, as they target 2 different use cases. I added the latest to the fixed row demo page (thanks for that one!) and it seems to work properly. Our use case is very similar to what this page shows, with a repeat within the row. Good guess!!
For resizeAndScrollHandler(), why are we forcing a $digest in the first place? When it does not update the DOM (handled by enqueueFetch), then it should just be a simple native scroll, shouldn't it? It is still conditioned by rowHeight, but I'm not sure that this is right :-)
I pushed that code to my branch.

@priandsf Answering to your question, I'm not sure about resizeAndScrollHandler $digest, I need some time to investigate it. But the question I would like to consider before is what we are going to improve? Browser reflow problem resulting from element.height() and maybe something else related to direct DOM manipulations OR $digest things? The answer is mostly important and requires demo-proof. I think we should not optimize things that really don't affect the performance. You know easier code could be more valuable than performant code, in case its performance bonus is negligible.

Fair enough - So far, resizeAndScrollHandler() in my app takes ~10ms, with most of the time within $digest when I scroll up and down without loading data. This can be higher when the GC is triggered. This is on a macbook pro running the latest Chrome. I haven't tried on other browsers yet...
So it doesn't feel laggy with this configuration. Up to you if you want/can remove the $digest.
Overall, the optimizations really made it smoother, thanks a lot for all your efforts. This library rocks!

@priandsf I have an answer on why do we need $digest in the end of resizeAndScrollHandler: it is needed for triggering topVisible and bottomVisible properties changing (after they are calculated by calculateProperties method) on the outer scope (top-visible="...") and to the outer scope adapter (adapter.topVisible). And we have two tests covering some of the correspondent use cases (this and this): they fail when that $digest is removed. And it does not depend on rowHeight. So when that $digest is removed, the assignments itself do work (scope and adapter props are being updated properly), but $watch does not work. The tests I just mentioned are about it.

One of the possible solutions is to html attribute for disable top/bottomVisible properties calculation/propagation + maybe adapter methods to enable/disable this feature runtime, and even the adapter method to calculate top/bottomVisible data immediately. So, users that don't need top/bottomVisible data (or don't need it right now) can switch this functionality off.

Thanks for the investigation, that makes sense. The flag will work for me, although it might complicate the developer experience as understanding the attribute requires a deep knowledge of the internals. Based on our performance result, it is fine for me if we postpone that change.
Are you willing to merge the other ones?

@dhilt I believe I got a solution for the $digest: the adapter checks if the directive has any of these attributes (top-visible, ...) defined and publishes it as a public property. Then the ui-scroll checks this property to know if it has to trigger the $digest.
I pushed the change to my branch. The unit tests and the demo (topVisible+topVisibleAdapter) work properly with this change. Let me know what you think!

…h=false" to the fixed scroll demo, for now...

dhilt · 2019-06-06T22:34:36Z

src/ui-scroll.js

            wrapper.unregisterVisibilityWatcher = wrapper.scope.$watch(() => visibilityWatcher(wrapper));
          }
-          if (!rowHeight) {
+          if (allowVisibilityWatch) {


This particular case, I mean hiding the elements before data binding, is not related to visibility-watcher. I would say that it may relate to rowHeight. We are hiding new rows before data binding, because the height might change after binding, and there could be some bad side effects (scroll bar shifting). And we may not hide new rows if we know that the height is not going to be changed.

dhilt · 2019-06-06T22:35:51Z

src/ui-scroll.js

@@ -397,7 +400,7 @@ angular.module('ui.scroll', [])
          // We need the item bindings to be processed before we can do adjustments
          !$scope.$$phase && !$rootScope.$$phase && $scope.$digest();

-          if (!rowHeight) {
+          if (allowVisibilityWatch) {


Showing new rows after binding relates to hiding new rows before binding. So it does not relate to visibility-watcher, it may relate to rowHeight (see previous comment).

priandsf · 2019-06-19T18:35:26Z

@dhilt We are intensively testing this optimization and found an issue with the allow-visibilty-watch flag.

=> How it manifests:
Scrolling with the mouse wheel does not work when the scroller is first rendered. After we scroll once using the scrollbar, then it starts to work properly

=> Why it behaves like this:
Because the wheelHandler() event handler checks for scrollTop === 0 && !buffer.bof. At that time, bof==false and it prevents the scroll (both directions).

=> Why it works with the visibility watcher
Because visibilityWatcher() calls doAdjust() which fetches the rows before first (negative index in my case). This sets bof=true. It seems to be a side effect here that makes the wheel work.

I'm not sure what wheelHandler() is for. I suppressed it for testing and didn't see an issue. But I bet there is a good reason for it :-)
A solution is to detect the scroll direction and add that to each condition, like bellow:

const deltaY = event.deltaY;
if ((deltaY<0 && scrollTop === 0 && !buffer.bof) || (deltaY>0 && scrollTop === yMax && !buffer.eof)) {

But checking deltaY to find the direction is not reliable, according to MDN.

Any idea?

dhilt · 2019-06-20T12:43:41Z

@priandsf There was the issue years ago (angular-ui/ui-utils#239) and it was fixed with PR Hill30/NGScroller#39, which is responsible for wheelHandler code. We also have demo http://localhost:5005/scrollBubblingPrevent/scrollBubblingPrevent.html and test case ("prevent unwanted scroll bubbling").

Well, I took v1.7.4, commented out

wrapper.unregisterVisibilityWatcher = wrapper.scope.$watch(() => visibilityWatcher(wrapper));

line and run the demo app. And I didn't see your problem with Mac's track pad (wheelHandler did work). I will try to reproduce your issue, but at first glance it's not so obvious, and it would be helpful if you might say how to reproduce it with minimal changes over the latest version (1.7.4).

priandsf · 2019-06-21T12:17:00Z

@dhilt Ok I narrowed down the problem in our app. I cannot reproduce it yet on one of the samples ;-(

To make a long story short, it happens because buffer.effectiveHeight in enqueueFetch returns 0. It returns 0 because the outerHeight of every element is 0, although the elements are properly inserted in the DOM but not yet "digested" (resolved: the ng-repeat is not yet executed). Hard to understand why, but our data retrieval code triggers some $digest which led to this situation.

Nevertheless, I checked-in a fix that makes it works in our case: when row-height is provided, the buffer uses it to calculate the effective height. This is consistent with rest of the code.

…tatic-row-height

# Conflicts: # dist/ui-scroll-grid.js # dist/ui-scroll.js # dist/ui-scroll.js.map # dist/ui-scroll.min.js # dist/ui-scroll.min.js.map # src/modules/buffer.js # src/modules/padding.js # src/modules/viewport.js # src/ui-scroll.js

priandsf · 2019-08-16T21:20:22Z

@dhilt Hi Denis, I'll be happy if you can have a look at the row-height. I put many optimizations into it:

faster height calculation
scroll debouncing
scrollTo() method to scroll to an absolute position
more resistant to a scroll frenzy, particularly when the rows are slow to render
We did some internal tests and so far we haven't found any issue.
I added a new sample (Datasource as a service with fixed row height) to show the performance differences when scrolling through a large dataset.
Let me know what you think.

priandsf · 2019-08-26T15:38:31Z

@dhilt I uploaded our latest code to my branch if you want to give a look. We did sone extensive testing on our application and we haven't found any major issue.

dhilt · 2019-08-27T14:03:10Z

@priandsf I will take a look when I have time, especially I think I'm most excited about the performance demo!

priandsf and others added 4 commits June 4, 2019 15:24

Added a row height parameter

61cb0cc

Made $digest optional in processUpdates() when the row height is fixed

bedd378

fixedRowHeight demo

71c2f33

release processUpdates

8f4bece

dhilt commented Jun 5, 2019

View reviewed changes

Separated visbiility watch & row height. Added "allow-visibility-watc…

551bff3

…h=false" to the fixed scroll demo, for now...

dhilt commented Jun 6, 2019

View reviewed changes

dhilt mentioned this pull request Jun 17, 2019

Changed the JQuery detection #222

Closed

priandsf and others added 4 commits June 19, 2019 13:35

Looks like the source was not properly commited - changed ui-scroll

43561dd

jquery load demo

d183273

v1.7.4 release

62c8bc6

v1.7.4 build

fdcca3d

Fix the outerHeight calculation in buffer when rowHeight is provided

b1be940

priandsf and others added 9 commits June 21, 2019 08:40

Merge branch 'master' of github.com:angular-ui/ui-scroll into Added-s…

45a8387

…tatic-row-height

Reverted bad commit on demo/fixedRowHeight for test purposes

95a6d4a

fix lodash dependency vulnerability

359d905

gh-pages auto deploy

b174ffc

travis.iml updates

0b62060

Added unbindEvent() to reload.

bbc1a74

Support for row-height

dd3bdfc

Merge branch 'master' into Added-static-row-height

3cff84e

# Conflicts: # dist/ui-scroll-grid.js # dist/ui-scroll.js # dist/ui-scroll.js.map # dist/ui-scroll.min.js # dist/ui-scroll.min.js.map # src/modules/buffer.js # src/modules/padding.js # src/modules/viewport.js # src/ui-scroll.js

compiled files

07e7047

Latest code dealing with row height.

9c26d38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed row height optimization #221

Fixed row height optimization #221

dhilt commented Jun 4, 2019 •

edited

Loading

dhilt Jun 5, 2019

dhilt Jun 5, 2019

dhilt Jun 5, 2019

priandsf Jun 6, 2019

dhilt Jun 6, 2019

priandsf Jun 7, 2019 •

edited

Loading

dhilt Jun 7, 2019

priandsf Jun 9, 2019

priandsf Jun 14, 2019

dhilt Jun 6, 2019

dhilt Jun 6, 2019

priandsf commented Jun 19, 2019

dhilt commented Jun 20, 2019 •

edited

Loading

priandsf commented Jun 21, 2019 •

edited

Loading

priandsf commented Aug 16, 2019

priandsf commented Aug 26, 2019

dhilt commented Aug 27, 2019

		@@ -386,8 +397,11 @@ angular.module('ui.scroll', [])
		// We need the item bindings to be processed before we can do adjustments
		!$scope.$$phase && !$rootScope.$$phase && $scope.$digest();

Fixed row height optimization #221

Are you sure you want to change the base?

Fixed row height optimization #221

Conversation

dhilt commented Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priandsf Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priandsf commented Jun 19, 2019

dhilt commented Jun 20, 2019 • edited Loading

priandsf commented Jun 21, 2019 • edited Loading

priandsf commented Aug 16, 2019

priandsf commented Aug 26, 2019

dhilt commented Aug 27, 2019

dhilt commented Jun 4, 2019 •

edited

Loading

priandsf Jun 7, 2019 •

edited

Loading

dhilt commented Jun 20, 2019 •

edited

Loading

priandsf commented Jun 21, 2019 •

edited

Loading