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

Possible performance issue with double forced reflow #761

Closed
PierreDuc opened this issue Jun 5, 2018 · 5 comments · Fixed by #773
Closed

Possible performance issue with double forced reflow #761

PierreDuc opened this issue Jun 5, 2018 · 5 comments · Fixed by #773
Assignees
Labels
has pr A PR has been created to address this issue P3 Important issue that needs to be resolved performance
Projects

Comments

@PierreDuc
Copy link

Bug, feature request, or proposal:

Bug/performance proposal

What is the expected behavior?

Not having two forced reflows on directive initialisation

What is the current behaviour?

Two (or more) forced reflows on directive initialisation

What are the steps to reproduce?

One example is in the fxLayoutDirective. You can see the _updateWithDirection being called inside the ngOnChanges and after that in the ngOnInit.

I might be wrong, but from what I can tell from the profiler this calls the heavy _updateWithDirection twice on initalisation. Because you initialise the directive by using the layout input, it will always go through the ngOnChanges hook anyways. Which makes this line obsolete, or better yet, causing unnecessary recalculations.

I noticed in the profiler that these updates are pretty intensive for the cpu, by causing forced reflows

Other examples:

FlexAlignDirective._updateWithValue
FlexOrderDirective._updateWithValue
FlexDirective._updateStyle
FlexLayoutAlignDirective._updateWithValue

Instead of removing the corresponding lines, you could also check inside the ngOnChanges if the ngOnInit hook has already ran. And if so, execute the function. This needs to be done for the LayoutGapDirective anyways, because it needs to be executed inside the ngAfterViewInit.

Another thing that I noticed is the cpu hungry _getDisplayStyle inside every directive that extends the BaseDirective. If the current element does not use the media queries .xs, .xl ..., how necessary is this call?

What is the use-case or motivation for changing an existing behaviour?

To have a faster flex framework

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

All

Is there anything else we should know?

Like I said, my assumptions might be wrong, and there is actually a valid reason for executing it twice.

@CaerusKaru CaerusKaru self-assigned this Jun 5, 2018
@CaerusKaru CaerusKaru added has pr A PR has been created to address this issue P3 Important issue that needs to be resolved and removed needs: investigation labels Jun 11, 2018
@CaerusKaru CaerusKaru added this to Pending in API Fixes via automation Jun 11, 2018
@CaerusKaru CaerusKaru added this to the v6.0.0-beta.17 milestone Jun 11, 2018
@CaerusKaru
Copy link
Member

@PierreDuc Thank you for tracking this down. I've removed the duplicate function calls in #773. As for the calls to _getDisplayStyle, can you provide a profiling breakdown of where it's called, and how it's affecting performance? It's called primarily to determine show/hide or if there is a flex-direction present on a parent container.

@CaerusKaru CaerusKaru moved this from Pending to In-Progress in API Fixes Jun 13, 2018
@PierreDuc
Copy link
Author

@CaerusKaru It's seems like that _display is only used in the ShowHideDirective, but it's set in the ngOnInit of every directive that extends the BaseDirective, by calling _getDisplayStyle. So perhaps it's better to move this call to just ShowHideDirective.

@CaerusKaru
Copy link
Member

Excellent suggestion, it's been moved in the latest commit on the PR.

API Fixes automation moved this from In-Progress to Done Jul 25, 2018
CaerusKaru added a commit that referenced this issue Jul 25, 2018
* remove duplicate initialization calls in most directives
* remove `getDisplayStyle` call from BaseDirective `ngOnInit`

Fixes #761
@PierreDuc
Copy link
Author

Nice, just an easy benchmark shows that the flex-layout library is now 75% faster 👍

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
has pr A PR has been created to address this issue P3 Important issue that needs to be resolved performance
Projects
API Fixes
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants