Skip to content

perf(slider): convert slider adapters to class objects #19985

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

Closed
wants to merge 12 commits into from

Conversation

ngwattcos
Copy link
Contributor

@ngwattcos ngwattcos commented Jul 15, 2020

Converts slider adapter to class object to reduce memory usage. Adds several methods to work around private fields and methods.

Edit: performance improvements should be comparable to #19980

@ngwattcos ngwattcos requested a review from devversion as a code owner July 15, 2020 03:17
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 15, 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.

Do we have any data of impact for this? Generally, I like this a lot, but would be nice to see if this is actually having a noticeable impact on performance.

@kseamon
Copy link
Collaborator

kseamon commented Jul 16, 2020

Do we have any data of impact for this? Generally, I like this a lot, but would be nice to see if this is actually having a noticeable impact on performance.

When I asked Scott to take this on, I saw in the Chrome memory profiler that at the very least we could save hundreds of bytes per instance by doing this. Scott is going to do some performance comparisons, so we'll see the effect on CPU time soon.

Converts slider adapter to class object to hopefully reduce memory usage. Changes access modifiers on private fields.
Changes access modifies to _ngZone, _foundation, _elmentRef, and _componentRef so that we won't neeed getter methods.
Removes unused _adapter reference
Adds back getter methods for private fields
@devversion
Copy link
Member

Should we hold off with this PR? The MDC team released a new slider implementation that we soon are going to wrap. This involves a completely different adapter and we'll need to make the adapter a noop anyway for now (to get our CI green until we wrap the new reworked slider MDC component). See: #20054

@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@andrewseguin
Copy link
Contributor

We will soon be taking out our dependency on the adapters provided by MDC since they won't be supported later. Thank you for your contribution, sorry we weren't able to land this

@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 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants