Skip to content

perf(ripple): optimize event registration #18633

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

Merged
merged 3 commits into from
Apr 9, 2020

Conversation

mleibman
Copy link
Contributor

@mleibman mleibman commented Feb 27, 2020

Improve scalability of Angular Material components.
When used in an app with a large and heavy UI with large numbers of material components, they can quickly become prohibitively expensive due to their heavy DOM structure and lots of up-front work.

Registering event listeners is slow, and gets progressively slower as the DOM tree grows and gets more complex. Each component with a ripple ends up registering 6 event listeners. Consider a 100-row table with 6 buttons per row. That's 3600 event listeners that need to be registered during rendering. This PR makes 2400 of them registered on-demand.

Note, that this is really a stop-gap solution to help improve performance in our application. A proper implementation should utilize a single document-level service that only registers one set of these event listeners, and uses event delegation to apply ripple effects.

Improve scalabiliity of Angular Material components.
When used in an app with a large and heavy UI with large numbers of material components (for example in table cells), they can quickly become prohibitively expensive due to their heavy DOM structure and lots of up-front work.

Each component with a ripple ends up registering 6 event listeners. This PR makes 4 of them registered on-demand.

Note, that this is really a stop-gap solution to help improve performance in our application. A proper implementation should utilize a single document-level service that only registers one set of these event listeners, and uses event delegation to apply ripple effects.
@mleibman mleibman requested a review from devversion as a code owner February 27, 2020 05:52
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 27, 2020
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.

this._registerEvents(pointerDownEvents);
}

/** Handles all registered events. */
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the @docs-private annotation to this JsDoc block? That will prevent it from appearing in our generated API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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 G This is is related to a Google internal issue pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Feb 27, 2020
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Looks great to me. Thanks!

});

if (this._pointerUpEventsRegistered) {
Copy link

Choose a reason for hiding this comment

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

the if statements is really needed here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It avoids unnecessary iteration and removeEventListener() calls.

@jelbourn jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Mar 25, 2020
@mmalerba mmalerba merged commit 9a16e60 into angular:master Apr 9, 2020
mmalerba pushed a commit that referenced this pull request Apr 14, 2020
* perf(ripple): optimize event registration

Improve scalabiliity of Angular Material components.
When used in an app with a large and heavy UI with large numbers of material components (for example in table cells), they can quickly become prohibitively expensive due to their heavy DOM structure and lots of up-front work.

Each component with a ripple ends up registering 6 event listeners. This PR makes 4 of them registered on-demand.

Note, that this is really a stop-gap solution to help improve performance in our application. A proper implementation should utilize a single document-level service that only registers one set of these event listeners, and uses event delegation to apply ripple effects.

* Update core.d.ts

* Add the @docs-private annotation.
@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 May 10, 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
None yet
Development

Successfully merging this pull request may close these issues.

6 participants