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

Add MaxResults config option with default 10 #81

Closed
wants to merge 5 commits into from

Conversation

t9md
Copy link
Contributor

@t9md t9md commented May 10, 2017

Description of the Change

  • When many commands are added, command-palette:toggle become non responsive.

  • This PR add MaxResults options with default 10, which make command-palette:toggle more responsive even after hundreds commands are added.

  • New MaxResults config option, with default value 10.

    • Control how many items to be added on select-list.
    • By limiting items to render to MaxResults, it reduce rendering time and make command-palette more responsive.
  • This PR introduce breaking change since it truncate items to 10 by default.

    • In previous release, all commands are added on select-list.
    • This difference become obvious only when user scroll items in the the select-list.
    • Since without scrolling, items just visible is just 7 in bundled UI themes(one-dark, atom-dark etc).
  • Because main use-case of command-palette is narrow items by fuzzy-search by keyboard, adding non-visible items is just add extra delay in most situation.

  • So I set it's default to 10.

  • Even if this breaking change was not acceptable, I want this PR merged with different default value.

Background

When many commands are registered via community packages, command-palette:toggle become a little sluggish.

  • E.g.
    • Install tons of packages.
    • Install packages which add hundreds of commands(such as vim-mode-plus).

How much improve responsiveness by this PRs.

Here is the result collected at this commit(44fd635) by following steps.

  1. From source.gfm editor, start command-palette by cmd-shift-p.
  2. Observe console.log output to see how much time it took to collect items, and render items.
  3. collect + render time is roughly the time command-palette shows up in UX perspective.
  • My PC's spec is here.
$ sysctl -n machdep.cpu.brand_string
Intel(R) Core(TM) i5-6267U CPU @ 2.90GHz
$ sw_vers
ProductName:    Mac OS X
ProductVersion: 10.12.4
BuildVersion:   16E195
add-max-results-option github/command-palette%

No community packages( Atoms default )

  • MaxResults: 10000( No limit )
collect: 16.730ms
render: 44.497ms
item length 375
-----------
collect: 13.186ms
render: 35.304ms
item length 375
-----------
collect: 11.761ms
render: 36.046ms
item length 375
-----------
  • MaxResult: 50
collect: 14.101ms
render: 3.281ms
item length 375
-----------
collect: 13.299ms
render: 2.925ms
item length 375
-----------
collect: 12.891ms
render: 3.091ms
item length 375
-----------
  • MaxResults: 10
collect: 13.229ms
render: 1.930ms
item length 375
-----------
collect: 13.957ms
render: 1.862ms
item length 375
-----------
collect: 13.206ms
render: 1.648ms
item length 375
-----------

Some packages installed

  • installed community packages
one-dark-vivid-syntax: 1.7.1
atomic-chrome: 0.3.0
choose-pane: 0.7.0
clip-history: 0.4.0
cursor-history: 0.10.1
linter: 2.1.4
linter-ui-default: 1.3.0
markdown-toc-auto: 0.7.0
quick-highlight: 0.10.0
vim-mode-plus: 0.92.0
vim-mode-plus-keymaps-for-surround: 0.2.1
recent-finder: 0.5.1
  • MaxResults: 10000( No limit )
collect: 22.079ms
render: 77.315ms
item length 846
-----------
collect: 24.637ms
render: 87.127ms
item length 846
-----------
collect: 23.875ms
render: 107.473ms
item length 846
-----------```
  • MaxResults: 50
collect: 24.011ms
render: 5.482ms
item length 848
-----------
collect: 20.187ms
render: 4.475ms
item length 848
-----------
collect: 22.080ms
render: 4.981ms
item length 848
-----------
  • MaxResults: 10
collect: 23.265ms
render: 2.192ms
item length 848
-----------
collect: 23.262ms
render: 1.897ms
item length 848
-----------
collect: 23.693ms
render: 1.849ms
item length 848
-----------

Alternate Designs

No alternate design I considered.

Benefits

command-palette:toggle get more responsive by default, I believe it also make overall Atom's performance impression better.

Possible Drawbacks

Results items on select-list is truncated to 10 items by default.
When user scroll over items on select-list, user can see only 10 commands( in previous release user could see all available commands ).

Applicable Issues

Fix #80
Related #35

@t9md t9md changed the title Add MaxResults config value with default 10 Add MaxResults config option with default 10 May 10, 2017
@jordwalke
Copy link

jordwalke commented May 10, 2017

This is great. One of the comments I hear about Atom is how it is sluggish - and the command palette is cited specifically. I then watch that user use Atom and I notice they have many items in the command palette. This would fix that issue, making sure that Atom leaves a great impression on the user.

It seems that around a max of 20 items, would receive most of the benefits. Do you think 20 might be a better default?

Separately - what can be done to make the "Collect" phase faster? It seems once you get into the range of 50 items max, the Collect phase starts to be the place with the biggest need for improvement. (Maybe someone else can think about that in another PR).

@t9md
Copy link
Contributor Author

t9md commented May 10, 2017

It seems that around a max of 20 items, would receive most of the benefits. Do you think 20 might be a better default?

What the situation? Like type some keystroke to narrow items and use mouse to scroll over items around 20 items?

Separately - what can be done to make the "Collect" phase faster? It seems once you get into the range of 50 items max, the Collect phase starts to be the place with the biggest need for improvement. (Maybe someone else can think about that in another PR).

IMO, this is difficult part, command collecting is essentially done by atom-core's command registry, and command-palette need to collect all available commands to filter matching command by fuzzy-match.

@jordwalke
Copy link

What the situation? Like type some keystroke to narrow items and use mouse to scroll over items around 20 items?

Yes something like that. It's likely rare, with 10 items to do so. But it might happen. 20 would be even more rare, and since there's not much perf difference between 10 and 20, maybe 20 is the better default? I don't feel strongly. I always type ahead to navigate everything (maybe I will scroll through six items max). My concerns was other peoples' workflows, not my own.

@t9md
Copy link
Contributor Author

t9md commented May 10, 2017

Thanks, Me too have no strong opinion as long as MaxResults was set 10-20 by default.

But have strong opinion to set it, to minimize possibility for new Atom user misunderstand "Atom is slow" by seeing command-palette's lag.

This is typically happens when pure-Vim user migrate to Atom.
Since they likely to install vim-mode-plus and command-palette get slow unless limiting MaxResults, it tend to lead unfair bad reputation "Atom is slow".

@jordwalke
Copy link

But have strong opinion to set it, to minimize possibility for new Atom user misunderstand "Atom is slow" by seeing command-palette's lag.

This is typically happens when pure-Vim user migrate to Atom.
Since they likely to install vim-mode-plus and command-palette get slow unless limiting MaxResults, it tend to lead unfair bad reputation "Atom is slow".

Yes, I've seen this happen!

@winstliu
Copy link
Contributor

winstliu commented May 10, 2017

Thought: maybe we could keep adding results as you scroll? Like what the Project Find results do now - it starts out with a few results but keeps adding more as you scroll. That way we also don't need to add a new config option.

@jordwalke
Copy link

@50Wliu I thought of that too, but the implementation is probably a little more complex at that point. Can we start with a config and then later add some infinite scrolling? :D

@rsese
Copy link

rsese commented Nov 1, 2017

Hey @t9md - I asked the team to take a look at this and related issues/PRs and while the general idea is good, the decision was to pursue the approach in #92 (where it looks like you're already involved in that conversation ⚡). Thanks as always for all your contributions ✌️

@rsese rsese closed this Nov 1, 2017
@t9md
Copy link
Contributor Author

t9md commented Nov 1, 2017

Agree to close this PRs.
This PR is to solve response lag when opening palette, I don't think #92 is direct answer for that.
Since after #92 is landed, it still depends on pkg author's decision(need explicitly hide command).

I still see adding item incrementally is ideal solution(value #81 (comment)).
But this should be taken care of by atom-select-list, not command-palette side.

Anyway I want to see if hiding command contribute to reduce performance lag, and will re-think incrementally adding item approach if necessary.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants