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

Cache element items on creation #94

Merged
merged 4 commits into from Nov 16, 2017

Conversation

Projects
None yet
2 participants
@jarle
Contributor

jarle commented Nov 15, 2017

Continuing from atom/atom-select-list/pull/21. The gist of it is: cetain performance issues in command-palette is caused by the fact that elementForItem is invoked for every item whenever the list state is updated, causing command-palette to create and recreate every DOM element more often than needed.

A simple solution to the problem, which is implemented in this PR, is to cache item elements on creation. Items are cached and added incrementally as the selection or query changes.

@50Wliu 50Wliu requested a review from nathansobo Nov 15, 2017

jarle added some commits Nov 15, 2017

@nathansobo nathansobo merged commit 5dfe294 into atom:master Nov 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 16, 2017

Contributor

👏 👏 👏 Bravo @jarle. Thanks for your dedication on this issue and being patient with my incomplete feedback. Sometimes it's a process of discovery. Glad you took this up. Thanks so much.

Contributor

nathansobo commented Nov 16, 2017

👏 👏 👏 Bravo @jarle. Thanks for your dedication on this issue and being patient with my incomplete feedback. Sometimes it's a process of discovery. Glad you took this up. Thanks so much.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 16, 2017

Contributor

Published as 0.42.1 and upgraded in atom/atom@377eeea.

Contributor

nathansobo commented Nov 16, 2017

Published as 0.42.1 and upgraded in atom/atom@377eeea.

@jarle

This comment has been minimized.

Show comment
Hide comment
@jarle

jarle Nov 16, 2017

Contributor

Thanks, I have appreciated the feedback and suggestions a lot! Happy to see this merged.

Contributor

jarle commented Nov 16, 2017

Thanks, I have appreciated the feedback and suggestions a lot! Happy to see this merged.

@jarle

This comment has been minimized.

Show comment
Hide comment
@jarle

jarle Nov 16, 2017

Contributor

So it seems that the element cache is not utilized at all between command-palette toggles. This seems to happen because the items array passed to the underlying selectList is regenerated every toggle, causing the old WeakMap item keys to be obsolete. A solution could be to switch the top-level WeakMap to a Map, where the underlying commandName would be used as key.

EDIT: After looking at this today, I think perhaps the garbage collection plays a significant role in making the WeakMap keys obsolete. The solution in #95 should should still be valid.

Contributor

jarle commented Nov 16, 2017

So it seems that the element cache is not utilized at all between command-palette toggles. This seems to happen because the items array passed to the underlying selectList is regenerated every toggle, causing the old WeakMap item keys to be obsolete. A solution could be to switch the top-level WeakMap to a Map, where the underlying commandName would be used as key.

EDIT: After looking at this today, I think perhaps the garbage collection plays a significant role in making the WeakMap keys obsolete. The solution in #95 should should still be valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment