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

perf(table) Coalesces style updates after style measurements to reduc… #19750

Merged
merged 18 commits into from
Jul 27, 2020

Conversation

kseamon
Copy link
Collaborator

@kseamon kseamon commented Jun 24, 2020

…e layout thrashing Exposes the CoalescedStyleScheduler for use by other related components in a table such as column resize.

This will complement #19739, but the following measurements are taken without it. Each of these reduces the impact of the other but they'll both result in some savings (especially this one). I'm exposing the coalesced style scheduler so that it can also be used by column resize et al to coalesce along with sticky styler.

Before:
image
94ms script + 46ms rendering

After:
image
69ms script + 22ms rendering
This is scrolled down a little too far to see the effect - we're now down to one compute styles + layout before data rows are rendered and one after. Previously there were many.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 24, 2020
import {from, Observable} from 'rxjs';

@Injectable()
export class CoalescedStyleScheduler {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUIC, this is essentially a step towards a more generic way of scheduling and coordinating DOM updates in a way that prevents layout thrashing, but it is missing some core functionality from that. This has already been implemented in other libraries such as Google Closure - https://github.com/google/closure-library/blob/master/closure/goog/dom/animationframe/animationframe.js. Perhaps we can port that for a more flexible/powerful tool?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Jeremy and co are up for it, I can move this to a more common part of the cdk and have it be providedIn: 'any' rather than scoped to a table. For the purposes of this PR, I'm not looking to turn it into a fully-featured library, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep it more tightly scoped for the time being. I believe this is something we should tackle at the framework level rather than at the library level, since then everyone can get the benefits without having to jump through extra hoops.

src/cdk/table/coalesced-style-scheduler.ts Outdated Show resolved Hide resolved
src/cdk/table/sticky-styler.ts Show resolved Hide resolved
tools/public_api_guard/cdk/table.d.ts Outdated Show resolved Hide resolved
@kseamon kseamon force-pushed the table-coalesce-measurements branch from 1929463 to 498ff4c Compare July 1, 2020 18:27
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release P2 The issue is important to a large percentage of users, with a workaround labels Jul 8, 2020
@jelbourn jelbourn added this to PRs in P2 PRs Jul 9, 2020
@mmalerba mmalerba added the G This is is related to a Google internal issue label Jul 14, 2020
@andrewseguin andrewseguin moved this from PRs to In Progress in P2 PRs Jul 17, 2020
@jelbourn jelbourn merged commit ef8fc4f into angular:master Jul 27, 2020
P2 PRs automation moved this from In Progress to Done Jul 27, 2020
jelbourn pushed a commit that referenced this pull request Jul 27, 2020
#19750)

* perf(table) Coalesces style updates after style measurements to reduce layout thrashing Exposes the CoalescedStyleScheduler for use by other related components in a table such as column resize.

* Add license

* Fix column resize tests

* Fixed mdc-table tests

* jsdoc

* more comments

* lint

* lint

* Added _

* lint

* -override

* update api

* prevent resource leaks

* api

* readonly

* remove changes that are part of #19739

* Change to onStable to work around downstream test failures

* api update

(cherry picked from commit ef8fc4f)
crisbeto added a commit to crisbeto/material2 that referenced this pull request Aug 26, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Aug 26, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
@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 Aug 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
P2 PRs
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants