Skip to content

Commit

Permalink
Fix performance regression when updating the UI
Browse files Browse the repository at this point in the history
Somehow the `selectedCount` and other stats were triggering re-renders
of the entire page, probably because each folder component was checking
whether to show "selected" or "non-selected" toolbar buttons, and the
`selectedCount` was recalculated every time a window or tab changed.

We can do this recalculation by not triggering downstream updates, which
is what the new `computedLazyEq()` ref type does.  (Incidentally, Vue
has something similar internally, but it is not exposed for public
consumption, sadly.)
  • Loading branch information
josh-berry committed Jun 17, 2023
1 parent 45b7956 commit f9d5c48
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 20 deletions.
19 changes: 6 additions & 13 deletions src/model/filtered-tree.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {computed, ref, shallowReadonly, watchEffect} from "vue";
import {computed, shallowReadonly} from "vue";

import {computedLazyEq} from "../util";
import type {TreeNode, TreeParent} from "./tree";

export type FilteredItem<
Expand Down Expand Up @@ -100,21 +101,17 @@ export class FilteredTree<P extends TreeParent<P, C>, C extends TreeNode<P, C>>
// can have a huge cascading effect downstream--instead, we only want to
// write to isMatching when the value actually changes, to avoid triggering
// re-renders unnecessarily.
const isMatching = ref(this.predicate(unfiltered));
watchEffect(() => {
const res = this.predicate(unfiltered);
if (isMatching.value !== res) isMatching.value = res;
});
const isMatching = computedLazyEq(() => this.predicate(unfiltered));

const hasMatchingChildren = computed(
const hasMatchingChildren = computedLazyEq(
() =>
!!children.value.find(
i =>
i.isMatching ||
("hasMatchingChildren" in i && i.hasMatchingChildren),
),
);
const filteredCount = computed(() => {
const filteredCount = computedLazyEq(() => {
let count = 0;
children.value.forEach(i => {
if (!i.isMatching) ++count;
Expand Down Expand Up @@ -163,11 +160,7 @@ export class FilteredTree<P extends TreeParent<P, C>, C extends TreeNode<P, C>>
// can have a huge cascading effect downstream--instead, we only want to
// write to isMatching when the value actually changes, to avoid triggering
// re-renders unnecessarily.
const isMatching = ref(this.predicate(unfiltered));
watchEffect(() => {
const res = this.predicate(unfiltered);
if (isMatching.value !== res) isMatching.value = res;
});
const isMatching = computedLazyEq(() => this.predicate(unfiltered));

// Ugh, I wish shallowReadonly() actually unwrapped the top-level refs...
const c = shallowReadonly({
Expand Down
10 changes: 6 additions & 4 deletions src/model/selection.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type {Ref} from "vue";
import {computed} from "vue";

import {computedLazyEq} from "../util";

/** A model which allows for its elements to be selected must implement a few
* things so that the selection model itself can report selected items in the
Expand Down Expand Up @@ -51,9 +52,7 @@ export interface SelectableModel<T = any> {
export class Model {
readonly models: readonly SelectableModel[];

readonly selectedCount = computed(() =>
this.models.reduce((count, m) => count + m.selectedCount.value, 0),
);
readonly selectedCount: Ref<number>;

/** The last item that was selected. */
lastSelected?: {
Expand All @@ -71,6 +70,9 @@ export class Model {
* they appear in the UI. */
constructor(models: SelectableModel[]) {
this.models = Array.from(models);
this.selectedCount = computedLazyEq(() =>
this.models.reduce((count, m) => count + m.selectedCount.value, 0),
);
}

/** Clear all selections, including any that may not be visible to the user.
Expand Down
7 changes: 4 additions & 3 deletions src/model/tabs.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import {computed, reactive, ref, watch, type Ref} from "vue";
import {reactive, ref, watch, type Ref} from "vue";
import type {Tabs, Windows} from "webextension-polyfill";
import browser from "webextension-polyfill";

import {
AsyncTaskQueue,
backingOff,
computedLazyEq,
expect,
filterMap,
shortPoll,
Expand Down Expand Up @@ -76,12 +77,12 @@ export class Model {
* precisely the same as `focusedWindow`, because it accounts for the fact
* that the user could tear off the Tab Stash tab into a new window, and yet
* still want to manage the window that the tab was originally opened in. */
readonly targetWindow = computed(
readonly targetWindow = computedLazyEq(
() => this.initialWindow.value ?? this.focusedWindow.value,
);

/** The number of selected tabs. */
readonly selectedCount = computed(() => {
readonly selectedCount = computedLazyEq(() => {
let count = 0;
for (const _ of this.selectedItems()) ++count;
return count;
Expand Down
14 changes: 14 additions & 0 deletions src/util/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,20 @@ export function cmpVersions(a: string, b: string): number {
return va.length - vb.length;
}

/** Like Vue's computed(), but it only triggers updates if the value has
* actually changed. "Changed" in this case means that `newValue !== oldValue`.
* This is generally more efficient than using computed() for primitive
* values--using it with objects/arrays is not recommended; it is likely to
* perform strictly worse than computed(). */
export function computedLazyEq<T>(f: () => T): Vue.Ref<T> {
const ref: Vue.Ref<T> = Vue.ref(undefined!);
Vue.watchEffect(() => {
const newValue = f();
if (ref.value !== newValue) ref.value = newValue;
});
return ref;
}

// Given a URL to stash, return the URL that is actually openable by an
// extension. This is needed because sometimes the saved URL is a "privileged"
// URL (e.g. a tab which is open in Reader Mode), which we won't be able to open
Expand Down

0 comments on commit f9d5c48

Please sign in to comment.