Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

RC4: new feat(showHide) has a huge performance impact #5760

Closed
maximethebault opened this issue Nov 16, 2015 · 6 comments
Closed

RC4: new feat(showHide) has a huge performance impact #5760

maximethebault opened this issue Nov 16, 2015 · 6 comments
Assignees
Milestone

Comments

@maximethebault
Copy link

On a page using lots of ng-show/ng-hide, RC4 has a monstrous performance impact. I could pinpoint the problem to this commit:
7355e58 (aka #5579)

Does Angular-Material provide a way to disable that feature easily? Maybe something similar to what Angular Aria does:

$ariaProvider.config({tabindex: false});

Could be something like

$materialProvider.config({showHide: false});
@Koslun
Copy link

Koslun commented Nov 16, 2015

A codepen, measurements or something else to more concretely demonstrate the "monstrous performance impact" is probably warranted to give a final judgement and prove how big of an issue this actually is. Further enabling the team to prioritize it accordingly.

Judging from the issue linked, disabling it completely will prevent future md-virtual-repeat functionality so would think it would either be reverted and later optimized, or just optimized without a revert.

@maximethebault
Copy link
Author

Context: a table with 12 columns, 50 rows. Each cell has a ng-show attached. So basically, 600 ng-hide.
With 7355e58, once the data is loaded, I end up with a 10 second page freeze (latest Chrome, powerful computer).
Without 7355e58, once the data is loaded, the page is immediatly responsive.

Even if you reduce the number of ng-show/ng-hide to a more reasonable number (I don't want to debate over the relevance of 600 ng-hide on one page), it's still noticeable. Wouldn't it be possible to restrict this new directive to the children of a md-virtual-repeat element (since it's where they're useful)? Or have a way to disable the directive altogether? It's unnecessarily affecting the performance of all elements at the moment.

@kseamon
Copy link
Contributor

kseamon commented Nov 24, 2015

Restricting it to children of virtual-reapeat would not be useful - the point of this is for when there's a virtual-repeat inside. Out of curiosity, what scenario sees hundreds of ng-hide/ng-show instances toggling at the same time? Are they siblings or inside of one another?

@kseamon
Copy link
Contributor

kseamon commented Nov 24, 2015

We can probably find a way to make it configurable, or at the very least, opt-outable on a per-node basis.

(I don't mean to derail the thread, but for a variety of reasons, would it be feasible to put your entire table in a single ng-if or ng-hide/show instead of having hundreds of repeated instances of the directive? That's a lot of bindings).

@maximethebault
Copy link
Author

It's a data table with inline-editing capabilities. The ng-show are basically there to toggle between the display & the edit version of the cell.

But I didn't create the issue to talk about my specific problem, I can find ways around it (of course, it becomes more verbose and less beautiful but I'll get over it!). The idea was to let you know that angular-material can add a more or less noticeable performance overhead just by being there. In an ideal world, you wouldn't expect any performance hit before starting to use at least one angular-material component :)

@ThomasBurleson ThomasBurleson added this to the 1.0-rc8 milestone Dec 2, 2015
@kseamon
Copy link
Contributor

kseamon commented Dec 2, 2015

As a short-term fix, consider ng-if.

I'm thinking for the actual fix, I will have the ng-show/hide directives listen for something like 'ng-resize'enable' which the virtual-repeater (or anything else that needs the events) will $emit at startup.

Only then will they register their $watches that $broadcast the $md-resize event. That should be much, much cheaper.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants