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

Debounce the layout function in the list component #1928

Conversation

YonatanKra
Copy link
Contributor

when removing an item.

Closes #1927

See my remarks blow regarding the outcome of this performance improvement, and proposed changes to this PR.

@google-cla google-cla bot added the cla: yes label Nov 2, 2020
@YonatanKra
Copy link
Contributor Author

Performance gains

The following measurements were taken for the test (test:debug, http://localhost:9876/debug.html?grep=mwc-list%3A%20mwc-list%20performance%20issue) before and after applying the change.
Before:
image
After:
image
Can see from the image that the remove process took a roughly 1/9 of the time after the change.

@YonatanKra
Copy link
Contributor Author

Caveat: The debounced updates are no longer part of the lists'updateComplete hook.

Can try to add it to the _getUpdateComplete somehow if needs be.

Another solution can be to allow the user to add a flag asyncRemoval or syncRemoval, and commence the layout accordingly.

Let me know if and how you'd like to proceed with this one.

Copy link
Contributor

@e111077 e111077 left a comment

Choose a reason for hiding this comment

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

please update CHANGELOG.md

packages/list/test/mwc-list.test.ts Outdated Show resolved Hide resolved
packages/list/mwc-list-base.ts Outdated Show resolved Hide resolved
@e111077
Copy link
Contributor

e111077 commented Nov 2, 2020

super thanks for the PR!

@e111077
Copy link
Contributor

e111077 commented Nov 2, 2020

This seems like a simple enough implementation so let's go with this for now. Part of our code submission process includes importing this code into google's monorepo, and google-wide tests should help us run into migration issues other users may also have; this is where we can determine whether we need a flag. Though hooking into getUpdateComplete might be a requirement

@e111077 e111077 self-assigned this Nov 2, 2020
@YonatanKra
Copy link
Contributor Author

@e111077
Thanks for the comments. I've addressed all of them.
Let me know if something's missing.

Copy link
Contributor

@e111077 e111077 left a comment

Choose a reason for hiding this comment

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

final stretch! Great work so far!

packages/list/mwc-list-base.ts Show resolved Hide resolved
packages/list/mwc-list-base.ts Outdated Show resolved Hide resolved
packages/list/test/mwc-list.test.ts Outdated Show resolved Hide resolved
@YonatanKra YonatanKra requested a review from e111077 November 4, 2020 06:37
packages/list/mwc-list-base.ts Outdated Show resolved Hide resolved
@YonatanKra YonatanKra force-pushed the fix-list-performance-issue branch from fd72948 to 5d1284e Compare November 5, 2020 03:37
packages/list/test/mwc-list.test.ts Outdated Show resolved Hide resolved
packages/list/mwc-list-base.ts Outdated Show resolved Hide resolved
packages/list/mwc-list-base.ts Outdated Show resolved Hide resolved
@YonatanKra YonatanKra force-pushed the fix-list-performance-issue branch from 536cc9a to 8d3b2e3 Compare November 5, 2020 04:34
@YonatanKra
Copy link
Contributor Author

Resolved all mentioned issues 😀

Copy link
Contributor

@e111077 e111077 left a comment

Choose a reason for hiding this comment

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

I'm also debugging a failure in a YouTube test that uses select. A little afraid it may be caused by the debounce vs a buffer, but it's still too early to tell

packages/list/test/mwc-list.test.ts Outdated Show resolved Hide resolved
@YonatanKra
Copy link
Contributor Author

I'm also debugging a failure in a YouTube test that uses select. A little afraid it may be caused by the debounce vs a buffer, but it's still too early to tell

It is a breaking change, as it changes the interface (from sync to async). So it might be this test assumes the remove event should have sync effect.

@YonatanKra YonatanKra requested a review from e111077 November 8, 2020 08:26
@e111077
Copy link
Contributor

e111077 commented Nov 9, 2020

Haven't forgotten about this; still trying to untangle their esoteric code

copybara-service bot pushed a commit that referenced this pull request Nov 12, 2020
when removing an item.

Closes #1927

See my remarks blow regarding the outcome of this performance improvement, and proposed changes to this PR.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#1928 from YonatanKra:fix-list-performance-issue d47cf23
PiperOrigin-RevId: 341937184
copybara-service bot pushed a commit that referenced this pull request Nov 12, 2020
when removing an item.

Closes #1927

See my remarks blow regarding the outcome of this performance improvement, and proposed changes to this PR.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#1928 from YonatanKra:fix-list-performance-issue d47cf23
PiperOrigin-RevId: 341937184
copybara-service bot pushed a commit that referenced this pull request Nov 13, 2020
when removing an item.

Closes #1927

See my remarks blow regarding the outcome of this performance improvement, and proposed changes to this PR.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#1928 from YonatanKra:fix-list-performance-issue d47cf23
PiperOrigin-RevId: 341937184
copybara-service bot pushed a commit that referenced this pull request Nov 16, 2020
when removing an item.

Closes #1927

See my remarks blow regarding the outcome of this performance improvement, and proposed changes to this PR.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#1928 from YonatanKra:fix-list-performance-issue d47cf23
PiperOrigin-RevId: 341937184
copybara-service bot pushed a commit that referenced this pull request Nov 17, 2020
when removing an item.

Closes #1927

See my remarks blow regarding the outcome of this performance improvement, and proposed changes to this PR.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#1928 from YonatanKra:fix-list-performance-issue d47cf23
PiperOrigin-RevId: 341937184
@copybara-service copybara-service bot merged commit 03e7e77 into material-components:master Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When list items are removed list.layout is called multiple times can causes a performance issue
2 participants