Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not call `setItems()` every time that the fuzzy finder is shown #373

Merged
merged 3 commits into from Mar 28, 2019

Conversation

Projects
None yet
2 participants
@rafeca
Copy link
Contributor

commented Mar 28, 2019

Description of the Change

This PR unblocks the work to use native-fuzzy to improve the filtering performance by mitigating one of its concerns.

Basically, before this PR the UI was blocked by the execution of the setItems() method every time the fuzzy finder was shown (and it delayed its appearance in the UI). This was not a big problem before because the setItems() method used to be quite fast, but with the addition of native-fuzzy we need to do more expensive things here (we need to serialize and pass the whole list of items to the native-fuzzy).

Alternate Designs

N/A

Benefits

  • Unblocking a big performance improvement.
  • Less work done every time the fuzzy finder is opened

Possible Drawbacks

N/A

Applicable Issues

#370

@rafeca rafeca self-assigned this Mar 28, 2019

@rafeca rafeca requested review from as-cii and jasonrudolph Mar 28, 2019

@@ -734,6 +747,9 @@ describe('FuzzyFinder', () => {
await projectView.toggle()

expect(PathLoader.startTask).toHaveBeenCalled()

await waitForReCrawlerToFinish(projectView)

This comment has been minimized.

Copy link
@rafeca

rafeca Mar 28, 2019

Author Contributor

There's a very small behaviour difference with this PR: before when adding or removing a project from the current Atom window the fuzzy finder was temporarily shown as empty while it was recrawling the new list of projects. Now, it displays the old results while the re-crawling happens (which is consistent with what happens when a new file is added/removed).

Before:

before

Now:

now

Based on its description, this test was not doing the assertion correctly: instead of checking that the end list of results was empty was just checking that the temporary loading window was empty (which also could cause some flakiness if the crawling happened faster than the time it takes to open the fuzzy finder), so I fixed it.

@rafeca rafeca changed the title Avoid setitems Do not call `setItems()` every time that the fuzzy finder is shown Mar 28, 2019

rafeca added some commits Mar 28, 2019

Avoid calling the setItems() method on every toggle
With this commit, the Fuzzy finder will only call the setItems() method
on its view whenever the items change (which happen whenever it
re-crawls the filesystem).

This commit will allow us to do more expensive operations on the
setItems() phase without blocking the UI when toggling the fuzzy finder.
Add new test case
This common scenario was not covered and it's easy to cause a regression
on it.

@rafeca rafeca force-pushed the avoid-setitems branch from 7d806a4 to d8b006a Mar 28, 2019

@rafeca rafeca referenced this pull request Mar 28, 2019

Closed

Improve performance when filtering large projects #370

1 of 1 task complete

@rafeca rafeca force-pushed the avoid-setitems branch from edaeb91 to e8c7d07 Mar 28, 2019

@rafeca rafeca merged commit 1467ce4 into master Mar 28, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafeca rafeca deleted the avoid-setitems branch Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.