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.

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

Improve Angular digest performance by watching dtOptions more shallowly #144

Closed
wants to merge 1 commit into from
Closed

Improve Angular digest performance by watching dtOptions more shallowly #144

wants to merge 1 commit into from

Conversation

Robinson7D
Copy link
Contributor

$watch with third argument set to true causes a deep search for changes on every $digest cycle. Meaning every time any Angular event happens (ng-clicks, etc.), the entire array, each of it's children, it's children's children, and so forth gets compared to a cached copy.

This seems unnecessary for dtOptions, and can cause some slowdown when dealing with very large datasets (or otherwise just if the dataset has a lot of deeply nested fields to traverse).

I left columns and columnDefs in full watch because they're typically much smaller, didn't seem as immediately important.

This change allowed several pages on a current project of mine to move from ~750ms to ~20ms digest cycles.


Flame charts from Chrome profiling to show the difference attached below:

Before:

screen shot 2014-12-09 at 10 44 42 am

After:

screen shot 2014-12-09 at 10 43 15 am

@Robinson7D
Copy link
Contributor Author

(Note that users can still manually trigger a reloadData if need to update deeply nested options, but that should almost never be necessary)

@l-lin
Copy link
Owner

l-lin commented Dec 9, 2014

Your contribution does not work for reloading data, changing the data or changing the options/columns 😢

@Robinson7D
Copy link
Contributor Author

Works for every table in my project, though when I change dtOptionsI already call .reloadData(). Columns and columnDefs are both entirely unchanged by this (both still use same old way).

@Robinson7D
Copy link
Contributor Author

Do you think it might be beneficial to have a boolean option for the directive that can enable/disable deep watching? Default it to true, but allow it to be toggled? I could add that, and I believe it would alleviate your worries and provide old behaviour by default while allowing watcher reduction :)

@Robinson7D
Copy link
Contributor Author

@l-lin Your concerns make sense to me, so I built the solution a little differently. The new code (if you look at the changes) is that by default everything works like it used to. All configuration options are watched deeply (with $watch(..., true)).

But I included an attribute so that if the directive has a truthy value for disable-deep-watchers at compile time then it will use $watchCollection(...) instead. This would allow users to prevent big datasets from thrashing Angular's $digest cycle at their own discretion (so, for example, if the user of this library does decide to disable-deep-watchers and wants to change a column's width, they either replace the columnDef object for the particular column, which would trigger $watchCollection, or they manually call reload after the changes).

But with these changes nothing changes unless the disable-deep-watchers attribute is set true, so nobody's old and existing code is affected. :)

l-lin added a commit that referenced this pull request Dec 10, 2014
@l-lin
Copy link
Owner

l-lin commented Dec 10, 2014

Awesome 👍
I change the attribute to have a dt- prefix. So the attribute will be dt-disable-deep-watchers.

@l-lin l-lin closed this Dec 10, 2014
@l-lin l-lin added this to the v0.3.1 milestone Dec 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants