Improve performance of quick bar by bramkragten · Pull Request #7359 · home-assistant/frontend

bramkragten · 2020-10-16T13:43:50Z

Proposed change

I noticed the quick bar could be pretty sluggish, added virtualization and some more improvements for speed (and some style).

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example configuration

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

donkawechico · 2020-10-16T16:55:43Z

I pulled this branch and tried locally with demo: enabled and I gotta be honest... This feels noticeably more sluggish to me :-\

I haven't examined the code yet, so I'll look at that and maybe put in some sort of timing mechanism

donkawechico · 2020-10-16T17:39:19Z

Here's what I'm talking about. Using devtools performance tab:

quick-bar-speed (3 paints / ~150ms)

dev branch (1 paint / ~55ms)

The same thing happens if I hit backspace and type the "h" again (testing caching).

Can you try the chrome perf tool locally and verify what I'm seeing?

donkawechico · 2020-10-17T20:40:45Z

Is it just me or does it now look like it has too much top padding? (I know it's the same padding, but maybe the icon is widening the gap between field and top?

donkawechico

As I was studying the code I realized a lot of this is going to change with the refactor PR. I think it would make more sense to review this once we merge the two and see how it all shakes out.

donkawechico · 2020-10-17T20:56:46Z

src/dialogs/quick-bar/ha-quick-bar.ts

    this._commandTriggered = -1;
+    this._commandItems = undefined;
+    this._entities = undefined;
+    this._filteredItems = undefined;


I don't think this is possible (see further down), but it occurred to me there could be a perf opportunity here:

The idea being: remove these undefined assignments, change showDialog() to reuse existing list, then change updated to invalidate the cache if it notices a New, Removed, or Renamed entity.

So like:

public async showDialog(params: QuickBarParams) { //... this._entities = this._entities || Object.keys(this.hass.states).map<HassEntity>( (entity_id) => this.hass.states[entity_id] ); //... public updated(changedProperties) { if ( changedProperties.has(_propertyThatIndicatesANewEntity) || changedProperties.has(_propertyThatIndicatesARemovedEntity) || changedProperties.has(_propertyThatIndicatesARenamedEntity ) { this._entities = undefined; } }

The reason I doubt this is feasible is that I'm not sure there's a simple property that indicates whether something was added... I think it's just a big list of existing states?

donkawechico · 2020-10-17T21:13:40Z

src/dialogs/quick-bar/ha-quick-bar.ts

-    this._itemFilter = "";
+    this._filter = "";
    this._commandTriggered = -1;
+    this._commandItems = undefined;


Minor:

We could probably remove this line and let the command list stay cached for next launch since the command list will very rarely change in the middle of a session.

zsarnett · 2020-10-20T15:45:29Z

src/dialogs/quick-bar/ha-quick-bar.ts

+import memoizeOne from "memoize-one";
+import "../../common/search/search-input";
+import { mdiConsoleLine } from "@mdi/js";
+import { scroll } from "lit-virtualizer";
+import { styleMap } from "lit-html/directives/style-map";


I see you stopped using the VScode auto organize imports 😜

src/dialogs/quick-bar/ha-quick-bar.ts

donkawechico · 2020-10-20T21:30:19Z

src/dialogs/quick-bar/ha-quick-bar.ts

+  private _handleSelected(ev: SingleSelectedEvent) {
+    const index = ev.detail.index;
+    const item = (ev.target as any).items[index].item;
+    this.processItemAndCloseDialog(item, index);
+  }


Is the purpose of this function just to separate the concept of "handling a selection" and "how to execute quick bar commands"?

it is to get the index and items from the event detail

But that was already being done inside processItemAndCloseDialog. Seems like an unnecessary abstraction to me.

donkawechico

I just found a bug where command items aren't executing even if you click. Styling is off too.

…ome-assistant-polymer into quick-bar-speed

donkawechico · 2020-10-21T23:49:15Z

src/dialogs/quick-bar/ha-quick-bar.ts

+
+        ha-icon {
+          color: var(--secondary-text-color);
+        }


@bramkragten Why is this ha-icon styling no longer in dev?

Current dev:

frontend/src/dialogs/quick-bar/ha-quick-bar.ts

Lines 338 to 358 in 07fc9b9

@media (min-width: 800px) {

ha-dialog {

--mdc-dialog-max-width: 800px;

--mdc-dialog-min-width: 500px;

--dialog-surface-position: fixed;

--dialog-surface-top: 40px;

--mdc-dialog-max-height: calc(100% - 72px);

}

}

ha-svg-icon.prefix {

margin: 8px;

}

.uni-virtualizer-host {

display: block;

position: relative;

contain: strict;

overflow: auto;

height: 100%;

}

I looked through git blame and the commit history of ha-quick-bar and I don't see anything that removed it O_o

It probably doesn't matter, icons look okay in dark mode.

But it does seem spooky that this code could vanish without a github trace.

I made a general PR to make it general

donkawechico · 2020-10-21T23:56:03Z

src/common/string/filter/sequence-matching.ts

        : fuzzySequentialMatch(filter, item.text);
      return item;
    })
+    .filter((item) => item.score === undefined || item.score > 0)


Why is this in here? This would return all invalid items.

Fixed here: #7430

That's your code 😅

lol, it is? 😆 Man I need to learn to read git history better. Sigh...

donkawechico · 2020-10-22T00:47:03Z

src/dialogs/quick-bar/ha-quick-bar.ts

-          value=${this._commandMode ? `>${this._itemFilter}` : this._itemFilter}
+          .filter=${this._commandMode ? `>${this._filter}` : this._filter}
          @keydown=${this._handleInputKeyDown}
-          @focus=${this._resetActivatedIndex}


Why was all the activated index stuff (styling the topmost and highlighted row) removed in a PR about perf? And without a review?

To me, that was a pretty helpful visual styling and would've liked to discuss in review context.

Because it didn't work anymore. Also it was using the wrong methods. If we would want to make the focus color different, we should change the css for that.

Fair enough!

donkawechico · 2020-10-22T01:09:34Z

src/dialogs/quick-bar/ha-quick-bar.ts

+        ${this._commandTriggered === index
+          ? html`<ha-circular-progress
+              size="small"
+              active
+              slot="meta"
+            ></ha-circular-progress>`


Moving the circular progress bar inside this async renderItem breaks the progress bar. We never go back into this function after this._commandTriggered gets a value so the progress bar never appears.

probot-home-assistant bot added the hacktoberfest label Oct 16, 2020

homeassistant added the cla-signed label Oct 16, 2020

donkawechico self-assigned this Oct 17, 2020

bramkragten requested a review from donkawechico October 17, 2020 20:48

bramkragten unassigned donkawechico Oct 17, 2020

donkawechico reviewed Oct 17, 2020

View reviewed changes

zsarnett reviewed Oct 20, 2020

View reviewed changes

src/dialogs/quick-bar/ha-quick-bar.ts Outdated Show resolved Hide resolved

bramkragten added 3 commits October 20, 2020 12:51

Improve performance of quick bar

2e3193f

Fix keyboard nav

b04c6b1

remove debounce

3f53143

donkawechico force-pushed the quick-bar-speed branch from f9b4522 to 3f53143 Compare October 20, 2020 20:24

Fix selected

aee4210

donkawechico approved these changes Oct 20, 2020

View reviewed changes

donkawechico requested changes Oct 20, 2020

View reviewed changes

bramkragten and others added 8 commits October 20, 2020 23:42

Set items after animation finished

b150741

Fix styling of virtualized list

fd41b10

Fix issue with first list item not being selected

21522b6

Merge branch 'quick-bar-speed' of https://github.com/home-assistant/h…

ed26ce0

…ome-assistant-polymer into quick-bar-speed

Tweaks

e8f8a14

Add debounce back

31441f2

Organize imports (for Zack)

7e0ce6f

Update ha-quick-bar.ts

4f09512

bramkragten merged commit 54ec379 into dev Oct 21, 2020

bramkragten deleted the quick-bar-speed branch October 21, 2020 11:07

bramkragten mentioned this pull request Oct 21, 2020

20201021.0 #7422

Merged

donkawechico reviewed Oct 21, 2020

View reviewed changes

donkawechico reviewed Oct 22, 2020

View reviewed changes

donkawechico mentioned this pull request Oct 22, 2020

Fix quick bar dark mode contrast, filter returning all items, no primary text #7430

Merged

9 tasks

donkawechico reviewed Oct 22, 2020

View reviewed changes

github-actions bot locked and limited conversation to collaborators Jul 5, 2022

	@media (min-width: 800px) {
	ha-dialog {
	--mdc-dialog-max-width: 800px;
	--mdc-dialog-min-width: 500px;
	--dialog-surface-position: fixed;
	--dialog-surface-top: 40px;
	--mdc-dialog-max-height: calc(100% - 72px);
	}
	}

	ha-svg-icon.prefix {
	margin: 8px;
	}

	.uni-virtualizer-host {
	display: block;
	position: relative;
	contain: strict;
	overflow: auto;
	height: 100%;
	}

Conversation

bramkragten commented Oct 16, 2020

Proposed change

Type of change

Example configuration

Additional information

Checklist

Uh oh!

donkawechico commented Oct 16, 2020

Uh oh!

donkawechico commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

quick-bar-speed (3 paints / ~150ms)

dev branch (1 paint / ~55ms)

Uh oh!

donkawechico commented Oct 17, 2020

Uh oh!

donkawechico left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donkawechico left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donkawechico Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramkragten Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

donkawechico commented Oct 16, 2020 •

edited

Loading

donkawechico Oct 22, 2020 •

edited

Loading

bramkragten Oct 22, 2020 •

edited

Loading