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

Add some small improvements to make the list load a bitfaster #41

Merged
merged 15 commits into from
Jun 6, 2024

Conversation

goanpeca
Copy link
Contributor

@goanpeca goanpeca commented May 29, 2024

Add small improvement on perceived speed of loading:

  • Use batches of 2 (bigger batches start making the UI block on my machine)
  • Use 62 ms between batches
  • Allow for searching of plugins not yet on list, and add them
  • throttle filter keystrokes so a filter starts only after some miliseconds
  • add (X/N) to both list when filtering so it is clear that X plugins are filtered out of a total of N
  • Change search type based on search text length, so if only start_with_char or less chars are typed then we match with starting letters only, otherwise we see if the text is contained in name/description etc. We also search for plugins that start with napari- plus the number of characters defined by start_with_char
Screenshot 2024-05-29 at 11 48 50 AM

@goanpeca goanpeca self-assigned this May 29, 2024
@goanpeca goanpeca requested a review from psobolewskiPhD May 29, 2024 16:49
@goanpeca goanpeca force-pushed the enh/speed branch 2 times, most recently from ee04247 to ddd8d4c Compare May 29, 2024 23:00
@psobolewskiPhD
Copy link
Member

<15 s to fully populate on my machine vs 45 on main!

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

For me this makes a marked difference and with showing how many are to be loaded makes it easy to estimate how long to wait.
filtering works well, thought I don't love the 200 ms pause -- left a question about that.

I'm going to approve, but hopefully one more person takes a look before merging.

@goanpeca
Copy link
Contributor Author

Thanks a lot @psobolewskiPhD for the review. You raised valid points and indeed the 200 can be lowered.

https://arxiv.org/pdf/1208.6109 so english average word is 5.1 letters, assuming a typing speed 60 word per minute (https://typing-speed.net/keystrokes-per-minute I tried this one and got 361 letters in 60 seconds, I am not a very fast typer hahah), so we can do some quick math and get:

5.1*60 + 60 = 366 letters in 60 seconds (the extra 60 is to account for spaces), so:
60 seconds / 366 letters =~ 0.163 seconds, which is pretty similar to what I got trying the other service.

So I think I have read somewhere that 150ms was a good number for interstroke time, I just bumped it a bit, but maybe 150 works after all :)

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 30, 2024

Ah, thanks for the explanation. So you're sort of debouncing.
What I notice is a delay between the first character and 2nd -- when the list is still loading.
Open the manager and try typing lung into the filter, all one word, naturally.
L will appear and then a delay and the rest.

I don't see this if I switch to main. And it's not due to the batch_size = 2.
This appears related to the filter timer, because I set it to 1 it's much better.
Is it possible the timer is triggering on first keystroke and then working properly after?

Maybe worth trying theqdebounced from superqt?
https://github.com/pyapp-kit/superqt/blob/7b1aefd119383ff3f87623a7ce997ec4d45b50f7/src/superqt/utils/_throttler.py#L379-L420

@goanpeca
Copy link
Contributor Author

Hi @psobolewskiPhD, yes, indeed debouncing basically, to avoid triggering the filter for just one letter, we could make sure to always have at least 2 letters or 3 before actually doing any filtering, that would also work.

Will take a look at superqt, I thought about it but did not test it.

Will check the first letter issue that you mention!

Thanks again for the comments and the review :)

@jaimergp
Copy link
Contributor

jaimergp commented Jun 4, 2024

Will check the first letter issue that you mention!

I've dealt with this kind of issue in the past (see UX for search at https://conda-forge.org/packages/) and resorted to the following strategy:

  • If len(query) == 1, then we only show results starting with that letter. I doubt folks want all packages containing a 🤷
  • Else, we show results containing it.

Depending on the volume of results for 1 (and 2?), maybe you can adjust the startsWith / contains threshold differently.

If this is too weird, then I suggest disabling search for queries of length == 1.

@goanpeca
Copy link
Contributor Author

goanpeca commented Jun 4, 2024

It was fairly simple to implement your suggestion @jaimergp , so I think we are good now with this PR :)

@Czaki
Copy link
Contributor

Czaki commented Jun 4, 2024

If len(query) == 1, then we only show results starting with that letter. I doubt folks want all packages containing a 🤷

It should also show packages starting from napari-a in our case.

@goanpeca
Copy link
Contributor Author

goanpeca commented Jun 4, 2024

It should also show packages starting from napari-a in our case.

Done @Czaki !

@psobolewskiPhD
Copy link
Member

Definitely feels better, but I still see a delay when typing a word like lung naturally, after the first letter.
Shouldn't the 120 ms timer would prevent anything after just the first letter (debouncing), until I finish? It feels like it need to trigger -- off the first letter -- and the debounces the rest of them.

@goanpeca
Copy link
Contributor Author

goanpeca commented Jun 5, 2024

Definitely feels better, but I still see a delay when typing a word like lung naturally, after the first letter.
Shouldn't the 120 ms timer would prevent anything after just the first letter (debouncing), until I finish? It feels like it need to trigger -- off the first letter -- and the debounces the rest of them.

Will check again, as I am not seeing that on my side 🤔

@psobolewskiPhD
Copy link
Member

Feels really solid now!
I do get a traceback when closing napari though:

Exception ignored in: <function CameraMonitor.__del__ at 0x1437a4180>
Traceback (most recent call last):
  File "/Users/piotrsobolewski/Dev/napari/napari/_qt/qt_viewer.py", line 1387, in __del__
    self._reset_camera_moving.cancel()
    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/piotrsobolewski/Dev/miniforge3/envs/napari-dev/lib/python3.12/site-packages/superqt/utils/_throttler.py", line 291, in __get__
    parent = self.parent()
             ^^^^^^^^^^^^^
RuntimeError: wrapped C/C++ object of type ThrottledCallable has been deleted

(open the manager, search for something, close the manager, quit napari)

Also, I was surprised to see the search persist through closing the widget, but I guess that's the caching from the other PR.
Honestly, not sure what the "right" behavior is, so not a blocker at all. Resolving the RunetimeError though would be good.

@goanpeca
Copy link
Contributor Author

goanpeca commented Jun 6, 2024

Hi @psobolewskiPhD thanks a lot for all the feedback. Hmmm weird, that traceback feels unrelated. But I will try to reproduce and see if soemthing i did messd it up!

Also, I was surprised to see the search persist through closing the widget, but I guess that's the caching from the other PR. Honestly, not sure what the "right" behavior is, so not a blocker at all. Resolving the RunetimeError though would be good.

Indeed! I can reset the search on reopen, that would probably feel more natural. Also working on a separate PR to add notifications when things ended or finished if for example you started installing something and closed the dialog. That way the user knows when something finished installing or if an error occured ✌🏼

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Jun 6, 2024

Shoot hang on, I could have had a branch checked out in napari repo. When I had this checked out. 😬

Edit: yup, sorry false alarm! I apologize I wasn't even thinking about my napari repo, even though to test this need main napari. Should have made a new env with a non-editable napari install!

@goanpeca
Copy link
Contributor Author

goanpeca commented Jun 6, 2024

@psobolewskiPhD now the filter gets cleared on hide/close to match your expectations, also added a small fix and now is good to go. Will merge after tests pass so I can focus #45 which is the final piece of work !

@goanpeca
Copy link
Contributor Author

goanpeca commented Jun 6, 2024

The failing test does not seem reproducible. Tested locally, will merge an iterate on the last PR

@goanpeca goanpeca merged commit bec5340 into napari:main Jun 6, 2024
9 of 10 checks passed
@goanpeca goanpeca deleted the enh/speed branch June 6, 2024 15:42
@goanpeca goanpeca added the enhancement New feature or request label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants