Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Remove element-cache #109

Merged
merged 1 commit into from
Feb 26, 2018
Merged

Remove element-cache #109

merged 1 commit into from
Feb 26, 2018

Conversation

t9md
Copy link
Contributor

@t9md t9md commented Feb 25, 2018

Description of the Change

Because I've got no response for #106.
Element-cache introduce unfix-able BUG.
And IMO no longer having real perf-gain by this feature now.
I will proceed to send this PR to remove element-cache I believe it should.

I'll explain some history/background for this feature. Pls ignore if you already agree to remove this feature.

Before render-on-visible PR

When #94 and #97 is implemented render-on-visible PR #101 had not yet be merged.

At that time command-palette did

  • When launch, render all items even if it's not visible in viewport(e.g. render 700 item elements when vim-mode-plus installed)
    • This make launch of command-pallette noticeably slow.
  • select-next/select-previous always re-render full item.

Because of such frequent full-item re-render behavior, element-cache introduced noticeable perf improvement for subsequent launch of command-palette:toggle(still very first launch was slow).

After render-on-visible PR

At that time command-palette does

  • When launch, just render 10 items regardless of actual item count. And render other items on visible timing.
    • So number of total registered command no longer have big impact for item rendering.
    • This make command-palette:toggle launch faster at very first invocation.
  • select-next/select-previous just render old and new selected item(two items).

Like this, chance of over hundreds full-item rendering is no longer happen.
So perf-gain where element-cache can introduce is no longer noticeable.

Here is Me and @jarle(implemented element-cache)'s discussion about this

#97 (comment)
#97 (comment)

Element cache is not fit in etch's VDOM diff based rendering

Element cache's bad impact was fixed in #105.
But still we can hit BUG introduced by element-cache as I described in #106.
And which is not controllable by command-palette side since it's depend on which pkg/command name to be used by user.
If commands start a char, it likely to become top of list, so more chance to hit this BUG.

Alternate Designs

I evaluated to keep seem-to-harmless humanized keystroke cache feature.
But I could not say noticeable diff so removed too.

Benefits

No unfixable exception.

Possible Drawbacks

None

Applicable Issues

@Ben3eeE Ben3eeE requested a review from daviwil February 26, 2018 07:57
Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @t9md!

@daviwil daviwil merged commit 496ac41 into atom:master Feb 26, 2018
@t9md
Copy link
Contributor Author

t9md commented Feb 26, 2018

Thanks.

Off topic.
Can you check #108? Which is more important than this PR, affecting every launch of palette in current beta.

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.

2 participants