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

Performance issue: EllipsisBehavior #902

Closed
Opiumtm opened this issue May 2, 2016 · 5 comments
Closed

Performance issue: EllipsisBehavior #902

Opiumtm opened this issue May 2, 2016 · 5 comments

Comments

@Opiumtm
Copy link
Member

Opiumtm commented May 2, 2016

image

Here screenshot from performance profiler.
After some navigation from pages EllipsisBehavior start to lag application especially on mobile devices.

It should be improved.

@Opiumtm
Copy link
Member Author

Opiumtm commented May 2, 2016

Again, plain hook on LayoutUpdated event is wrong. It become a disaster when page layout is changed frequently. It's a case when there is a ListView on page. ListView is virtualizing items control, so it create and destroy list items on demand. Layout is changed any time list is scrolled. If there is multi-phased rendering in ListView items for scroll optimization - things are going even worse. Layout updated not only on item rendering but also on every rendering phase.

I think it's fundamentally wrong to hook plainly on LayoutUpdated event in T10 library.

@Opiumtm
Copy link
Member Author

Opiumtm commented May 2, 2016

Here is why hook on LayoutUpdated and do some expensive work (like iterating over visual tree to find "More" button) is fundamentally wrong.

https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.xaml.frameworkelement.layoutupdated

When you handle LayoutUpdated, do not rely on the sender value. For LayoutUpdated, sender is always null, regardless of where the handler is attached. This is to prevent handlers from assigning any meaning to sender, such as implying that it was that specific element that fired the event out of the visual tree. LayoutUpdated implies that something in the overall visual tree has changed, and each specific object anywhere in the tree has the option of handling this occurrence. If you're familiar with lower-level render API design, you can equate LayoutUpdated being fired as similar to a "redraw needed" flag being set as part of an object-driven, retained-mode rendering logic.
Because LayoutUpdated fires in many circumstances and isn't always specific to an object that actually changes, consider whether handling the SizeChanged event instead is more appropriate for your scenario.

@Opiumtm
Copy link
Member Author

Opiumtm commented May 2, 2016

@JerryNixon look at it, it seems like a technical flaw in T10 library design
To prevent poor performance another approach needed in controls and behaviors. NavButtonBehavior had similar issue and also because of LayoutUpdated event

Opiumtm added a commit to Opiumtm/Template10 that referenced this issue May 3, 2016
@Opiumtm
Copy link
Member Author

Opiumtm commented May 3, 2016

@JerryNixon Fixed problem in #906 pull request. It greatly optimize performance of "ellipsis" button search in EllipsisBehavior.

For most common use-case of behavior (with PageHeader control - implying most people use PageHeader with T10) I used some architecturally unpleasant hack (cache button reference in private field and allow access to it via internal method for same-assembly EllipsisBehavior class). It somewhat smells but performance optimized.

If behavior will be used with other CommandBar descendant, optimized method of button search in visual tree. Replaced manual visual tree walk with obvious call to FindName on templated visual tree root. Only 3 WinRT interop calls instead of plenty WinRT calls in XamlUtils.AllChildren<Control>(commandBar).

But, it does not solve whole problem with LayoutUpdated event handling. I don't understand why LayoutUpdated event is used. I logically assume it's used to "automatically" catch primary and secondary command buttons visibility for Auto behavior mode. It's simple but fundamentally flawed from performance perspective. Maybe there is other reason for LayoutUpdated event which I do not know. Anyway, this pull request is only a partial solution which fix blatant performance degradation because of expensive manual tree walk on every LayoutUpdated event.

As I observed in debugger, LayoutUpdated event really raised on every visual tree change at page, no matter where. In many dynamic UI and virtualizing ListView scenarios it turn into a complete disaster.

As I posted above with screenshot from performance profiler, original LayoutUpdated handler used more CPU time than complex custom markup rendering engine used. If you look at that rendering engine - it parse potentially complex markup tree (with links, quotes, font styles and so on), calculate placement of text blocks and graphic primitives on canvas and insert these visual tree elements on canvas, engine partially written in C++ for better WinRT interop performance. And, such a complex task consume less CPU time than merely LayoutUpdated event handler!. Just imagine scale of disaster. Even on top tier Lumia 950 it works laggy and unpredictable with dynamic UI and multi-phased ListView scenarios. On Lumia 920 it visually suffocated.

Usage of LayoutUpdated event handling should be validated. Why it is used?

@Opiumtm
Copy link
Member Author

Opiumtm commented May 3, 2016

And, I must add to said above.
LayoutUpdated event is raised at system layout update cycle.
Expensive work (such as manual visual tree walk) inside layout update cycle ruin UI smoothness and responsiveness.

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