Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed row height optimization #221

Open
wants to merge 20 commits into
base: fixed-height-optimization
Choose a base branch
from
Open

Fixed row height optimization #221

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
61cb0cc
Added a row height parameter
priandsf Jun 4, 2019
bedd378
Made $digest optional in processUpdates() when the row height is fixed
priandsf Jun 4, 2019
71c2f33
fixedRowHeight demo
dhilt Jun 5, 2019
8f4bece
release processUpdates
dhilt Jun 5, 2019
551bff3
Separated visbiility watch & row height. Added "allow-visibility-watc…
priandsf Jun 6, 2019
43561dd
Looks like the source was not properly commited - changed ui-scroll
priandsf Jun 17, 2019
d183273
jquery load demo
dhilt Jun 19, 2019
62c8bc6
v1.7.4 release
dhilt Jun 19, 2019
fdcca3d
v1.7.4 build
dhilt Jun 19, 2019
b1be940
Fix the outerHeight calculation in buffer when rowHeight is provided
priandsf Jun 21, 2019
45a8387
Merge branch 'master' of github.com:angular-ui/ui-scroll into Added-s…
priandsf Jun 21, 2019
95a6d4a
Reverted bad commit on demo/fixedRowHeight for test purposes
priandsf Jun 21, 2019
359d905
fix lodash dependency vulnerability
dhilt Jul 16, 2019
b174ffc
gh-pages auto deploy
dhilt Jul 16, 2019
0b62060
travis.iml updates
dhilt Jul 16, 2019
bbc1a74
Added unbindEvent() to reload.
priandsf Jul 31, 2019
dd3bdfc
Support for row-height
priandsf Aug 16, 2019
3cff84e
Merge branch 'master' into Added-static-row-height
priandsf Aug 16, 2019
07e7047
compiled files
priandsf Aug 16, 2019
9c26d38
Latest code dealing with row height.
priandsf Aug 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion demo/fixedRowHeight/fixedRowHeight.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ <h1 class="page-header page-header-exapmle">Fixed Row Height</h1>
</div>

<div class="viewport" ui-scroll-viewport>
<div ui-scroll="item in datasource" buffer-size="10" row-height="25" adapter="adapter">
<div ui-scroll="item in datasource" buffer-size="10" row-height="25" allow-visibility-watch="false" adapter="adapter">
<div class="item" ng-style="{'height': '25px'}">

{{item.text}}
Expand Down
2 changes: 1 addition & 1 deletion dist/ui-scroll-grid.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 18 additions & 17 deletions dist/ui-scroll.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/ui-scroll.js.map
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/ui-scroll.min.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/ui-scroll.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 7 additions & 4 deletions src/ui-scroll.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ angular.module('ui.scroll', [])
//
const rowHeight = parseNumericAttr($attr.rowHeight, null, false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// PHIL: Read the visibility watch option, true by default
const allowVisibilityWatch = $attr.allowVisibilityWatch!=='false';

// Revision IDs
//
let ridActual = 0; // current data revision id
Expand Down Expand Up @@ -259,7 +262,7 @@ angular.module('ui.scroll', [])
}

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

function visibilityWatcher(wrapper) {
Expand All @@ -278,10 +281,10 @@ angular.module('ui.scroll', [])

function insertWrapperContent(wrapper, insertAfter) {
createElement(wrapper, insertAfter, viewport.insertElement);
if (!rowHeight && !isElementVisible(wrapper)) {
if (allowVisibilityWatch && !isElementVisible(wrapper)) {
wrapper.unregisterVisibilityWatcher = wrapper.scope.$watch(() => visibilityWatcher(wrapper));
}
if (!rowHeight) {
if (allowVisibilityWatch) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

elementRoutines.hideElement(wrapper); // hide inserted elements before data binding
}
}
Expand Down Expand Up @@ -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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@priandsf priandsf Jun 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!
scroll-digest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!


if (!rowHeight) {
if (allowVisibilityWatch) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

updates.inserted.forEach(w => elementRoutines.showElement(w));
updates.prepended.forEach(w => elementRoutines.showElement(w));
}
Expand Down