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

Fix Case Event List Item and Event Form List Item templates from rerendering constantly creating DOM Nodes leak #2000

Closed
rjcorwin opened this issue Mar 13, 2020 · 8 comments
Assignees
Milestone

Comments

@rjcorwin
Copy link
Contributor

rjcorwin commented Mar 13, 2020

If you load up a case with a couple of events, you'll notice about every 5 seconds the Case Event List Item and Event Form List rerender resulting in an ever growing DOM nodes that take up memory. Note the flashing parts of the DOM here indicating a change in the DOM... When nothing has actually changed in the Angular component's data.

Mar-13-2020 12-04-45

I placed a debugger statement in those components, the function to actually render is not getting called. It's just the Angular template recreating those things at a constant interval. Those things created are actually template variables of unsanitized HTML. I imagine this recreation would also be true for text strings. Perhaps Angular is thrashing on HTML that when it renders changes itself and it keeps trying t change it back to the original value?

@rjcorwin rjcorwin added this to the v3.8.1 milestone Mar 13, 2020
@rjcorwin
Copy link
Contributor Author

We might be able to prevent this by disabling change detection on those Angular components with this strategy. However, that might be too high level, this is that particular template variables going through pipes keep rerendering. That unsanitizeHTML pipe might be the problem because it keeps piping out at this interval.

@rjcorwin
Copy link
Contributor Author

A note on the implications of this leak.. Leaving the app open on a list of event for a half hour results in 100% CPU usage and 1.5 million DOM Nodes. On a Samsung tablet, trying to navigate to an event takes about a minute or two.

Screen Shot 2020-03-13 at 12 21 14 PM

@mfinholt
Copy link
Member

Amazing Sleuthing, @rjsteinert !

@rjcorwin rjcorwin self-assigned this Mar 13, 2020
@rjcorwin
Copy link
Contributor Author

Thanks @mfinholt!

Turning off Angular's change detection is doing the trick! No more flashing changes in the DOM there.

Mar-13-2020 15-02-21

However DOM Nodes are still going up because of another component we need to turn off change detection on... The Case Component handling that description of the case.

Mar-13-2020 15-01-58

Meanwhile over on the list of Event Forms, the change detection seems to be getting more aggressive. It's flashing multiple times a second as opposed to the every 5 seconds we saw before.

Mar-13-2020 15-02-43

@rjcorwin
Copy link
Contributor Author

With manual change detection implemented, all is quiet on the Case page now on PWA with the recommended amount of DOM nodes for a page and 0 CPU usage.
Mar-13-2020 16-38-12

However on the APK, reducing the noise from these unnecessary refreshes has shone a light on something else that is consuming CPU, however not enough CPU to cause the significant slow downs we have seen. This may be due to the fact there is 10,000 cases on the APK install I have and something else is going on. More work to be done there but a different bug I'm assuming.

@rjcorwin
Copy link
Contributor Author

Oops, it's not the "optimal DOM nodes" level. The recommendation states 1,500 is optimal, we're at 6,200. But at least much better than the 132,000 captured in one of the gifs above.

rjcorwin added a commit that referenced this issue Mar 16, 2020
…-and-event-form-dom-leak

(for #2000) Fix DOM leaks in components Case, CaseEventListItem, and EventFormListItem
@rjcorwin rjcorwin added review In a prerelease for review. and removed in progress labels Mar 16, 2020
@rjcorwin
Copy link
Contributor Author

Fix confirmed by Sante Mali.

@rjcorwin rjcorwin removed the review In a prerelease for review. label Mar 25, 2020
@rjcorwin
Copy link
Contributor Author

Stashing some docs on finding DOM node leaks. I didn't end up finding them useful as I think they are outdated but worth keeping around in case we can find updated docs in the future.

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

No branches or pull requests

2 participants