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(cdk/table): Use afterRender hooks #28354

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

kseamon
Copy link
Collaborator

@kseamon kseamon commented Jan 2, 2024

Per Chrome devtools, in the Angular Material devapp, cuts down startup rendering time by about 60ms (~25%). In a real app screen with a single large table, the reduction is ~16%. The main difference is that the change makes a bigger difference when multiple tables are rendering at the same time, since previously separate CoalescedStyleScheduler instances would be grouping tasks separately per-table. Seems to reduce scripting time a smidge as well.

@kseamon kseamon changed the title perf(table): Use afterRender hooks perf(cdk/table): Use afterRender hooks Jan 2, 2024
@kseamon kseamon force-pushed the table-afterRender branch from 9a6d0c4 to 0235c85 Compare January 2, 2024 18:16
@kseamon kseamon marked this pull request as ready for review January 2, 2024 19:07
@kseamon kseamon requested a review from andrewseguin as a code owner January 2, 2024 19:07
@kseamon kseamon added the G This is is related to a Google internal issue label Jan 2, 2024
@kseamon kseamon requested a review from crisbeto January 5, 2024 16:48
@kseamon kseamon force-pushed the table-afterRender branch from 0235c85 to d5f5cc1 Compare January 9, 2024 19:04
@kseamon kseamon force-pushed the table-afterRender branch 2 times, most recently from 568ccea to 77491f4 Compare January 18, 2024 20:45
private readonly _writeTasks: (() => unknown)[] = [];
private readonly _readTasks: (() => unknown)[] = [];

constructor(private readonly _ngZone: NgZone) {
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK afterRender has its own internal coalescing mechanism. Couldn't we switch to using afterRender directly in the table and remove the _CoalescedStyleScheduler altogether?

Copy link
Collaborator Author

@kseamon kseamon Jan 19, 2024

Choose a reason for hiding this comment

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

Not really possible alas as afterRender has a few limitations.

  1. It can only be invoked during injection context, wheras I need to have things happen at arbitrary times, and to be re-entrant.
  2. afterRender has no mechanism to flush outside of NgZone/CD, which is needed sometimes in the table.
  3. I tried to move columnResize off of the old _CoalescedStyleScheduler hooks, but could not get that to work, so is a blocker for now at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can only be invoked during injection context, wheras I need to have things happen at arbitrary times, and to be re-entrant.

This shouldn't be an issue because you can pass an injector instance to afterRender.

afterRender has no mechanism to flush outside of NgZone/CD, which is needed sometimes in the table.

Is there any reason to not just trigger applicationRef.tick imperatively in these cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

applicationRef.tick has additional side effects performance-wise. Given there are no changes to application state, just CSS, that does not seem like the best choice here.

Also, question on afterRender() - can you call safely afterRender/afterNextRender during afterRender? There are cases where it needs to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

If/when you get to trying to submit this change again, I'd love to discuss this more. afterRender is still in developer preview and its behavior is being tuned to ensure it satisfies required use-cases.

afterRender has no mechanism to flush outside of NgZone/CD, which is needed sometimes in the table.
...
applicationRef.tick has additional side effects performance-wise. Given there are no changes to application state, just CSS, that does not seem like the best choice here.
...
Given there are no changes to application state, just CSS, that does not seem like the best choice here.

We're working on several things that can help address this:

  1. Guaranteeing that render hooks run, even when no change detection is scheduled: angular/angular@80b953a
  2. Not refreshing views if we only need to flush afterRender hooks: angular/angular@51dfdbb
  3. Things that notify the scheduler (like afterRender hooks) should work when outside the zone (even in applications that use ZoneJS): signal updates and markForCheck should always schedule change detection angular#53844

src/cdk/table/sticky-styler.ts Outdated Show resolved Hide resolved
src/cdk/table/coalesced-style-scheduler.ts Show resolved Hide resolved
@andrewseguin andrewseguin added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jan 22, 2024
@andrewseguin andrewseguin merged commit 81cb5ac into angular:main Jan 22, 2024
25 of 27 checks passed
andrewseguin pushed a commit that referenced this pull request Jan 22, 2024
andrewseguin added a commit to andrewseguin/components that referenced this pull request Jan 22, 2024
crisbeto pushed a commit that referenced this pull request Jan 23, 2024
crisbeto pushed a commit that referenced this pull request Jan 23, 2024
@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 Feb 26, 2024
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 G This is is related to a Google internal issue target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants