Skip to content
This repository has been archived by the owner on Aug 11, 2024. It is now read-only.

Update search to use ViewStore #934

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Update search to use ViewStore #934

merged 1 commit into from
Oct 16, 2023

Conversation

gordonbrander
Copy link
Collaborator

@gordonbrander gordonbrander commented Oct 13, 2023

PR updates search view to use a ViewStore. In my playtesting, this significantly improves the perceived perf of type-ahead suggestions.

Why? I suspect this may have something to do with the way inputs work in SwiftUI. Perhaps the binding-style closer access that goes all the way up to store allows the input field to render text BEFORE the search view is invalidated by the parent.

Background:

  • I've been bothered by the perceived performance of type-ahead search in the app.
  • At first, I suspected this was due to SQLite full text search running up against large amounts of indexed peer content. So I tried searching only post titles instead of full text search.
    • However, upon investigating the app container, it turned out I was searching across relatively low amounts of content
    • SQLite was not the bottleneck.
  • I also tried limiting the amount of serializing needed for logging by implementing a CustomStringConvertible for SearchAction. This wasn't the bottleneck, but I kept it anyway, because IMO its probably a good idea not to serialize and log all suggestions at every keystroke.
  • The improvement came with using ViewStore. Surprising, but then again, perhaps not, given the issues we have had with input fields in the past that were fixed by ViewStore.

In my playtesting, this significantly improves the perf of type-ahead
suggestions. I suspect this may have something to do with the way inputs
work in SwiftUI. Perhaps the binding-style closer access that goes all
the way up to store allows the input field to render text BEFORE the
search view is invalidated by the parent.
Copy link
Collaborator

@bfollington bfollington left a comment

Choose a reason for hiding this comment

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

Wow, I'm surprised it makes such a difference but that's good news. In my mind this bumps the priority on refactoring other state + send to ViewStore cases a little... Even if nothing else is showing performance issues yet.

Playtested on the simulator and I can feel the difference with 100 notes, it looks almost as if we're rendering half as often now, there's a flicker on main when re-rendering compared to this PR.

@bfollington bfollington merged commit 319c942 into main Oct 16, 2023
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.

2 participants