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

smooth one-shot finder #1853

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dkuettel
Copy link

@dkuettel dkuettel commented Apr 18, 2022

I'm trying to improve responsiveness and smoothness of the user experience if you have slow one-shot finders. Slow because the command you run is slow, not slow because the actual lua code is heavy.

If you use a finder like this

            finder = finders.new_oneshot_job({ "test-telescope" }, {
                entry_maker = function(line)
                    return {
                        value = line,
                        display = line,
                        ordinal = line,
                        path = "./some.py",
                        lnum = 12,
                    }
                end,
            })

and you have this script in your path

#!/bin/zsh
set -eu -o pipefail

for i in $(seq 100); do
    sleep 0.15s
    if [[ $(( $i % 10 )) == 0 ]]; then
        echo $i signal
    else
        echo $i noise
    fi
done

then, on current master, you'll notice a bumpy ride:

  • If you just wait, and dont type, the status counter is counting up, but you dont see any entries in the results window. Unless you have the picker in dropdown mode.
  • If you start typing a search expression, results dont filter until after the command has finished. There is no incremental search results.
  • The status counter updates only when there is a result that's filtered in, it doesnt update when there is a new entry that is not shown.
  • Even after the command has actually finished, and you change the search expression, the result buffer updates with a mix of new and old results and only eventually shows a clean list. However that is only really visible if the finder lua code is slow, not the command. After the command has finished, the one-shot finder has all entries cached and it's probably reloaded too fast to notice unless you make it slow on purpose.

There is still a pretty big TODO comment in async_oneshot_finder.lua where I'm still a bit lost. Overall I'm not very robust on plenary, libuv, and the whole async stuff. But I noticed that sometimes process_results(...) fails because it tries inside to call vim.api functions and that is only allowed (?) when we are on the main event-loop. Originally async.util.scheduler() was only called every 1000 entries or so. But if I do that I occasionally get the problem with vim.api calls. I dont quite understand what the scheduler() is doing.

@dkuettel dkuettel changed the title Dk/smooth one shot smooth one-shot finder Apr 18, 2022
@Conni2461
Copy link
Member

Conni2461 commented Apr 18, 2022

This problems is a problem with sorting_strategy="descending" (which is the default strategy). It works find with sorting_strategy="ascending"

I dont wanna comment on this prior to #1491 (just tested it with that PR and it also seems to be fixed using that branch). This PR completely changes the rendering.

Thanks for the PR thought :)

CC @tjdevries

@dkuettel
Copy link
Author

dkuettel commented Apr 18, 2022

This problems is a problem with sorting_strategy="descending" (which is the default strategy). It works find with sorting_strategy="ascending"

I dont wanna comment on this prior to #1491 (just tested it with that PR and it also seems to be fixed using that branch). This PR completely changes the rendering.

Thanks for the PR thought :)

CC @tjdevries

I've seen #1491 :) but I dont know how soon it will be on master.

Indeed the small fix I made is because of ascending vs descending. And this fix might then soon be obsolete. And admittedly very ad-hoc and messy.

However, the changes on async_oneshot_finder.lua, where I need some feedback on, will probably still make sense, independent of the "rendering engine". Unless #1491 will also change the interface between the picker and the finder?

-- TODO ok something is very wrong here, scheduler waits until we are on the main thread? then we always need it
-- or should process_result do it itself, because they want to call vim.api stuff
-- original did something like every 1000 iterations, but that makes no sense because you dont know how fast is your command
-- how does this relate to await_schedule()? ah the same. just an alias
Copy link
Member

Choose a reason for hiding this comment

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

the reason the old one did every 1000 iterations was because it was expensive to do it every single time for something that might be called a million times.

We could do some testing to see how this fairs, but I'm hesitant to change it.

Copy link
Author

@dkuettel dkuettel Apr 19, 2022

Choose a reason for hiding this comment

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

When I tested it with the above script producing 1M entries, it was still super fast and very responsive, but my current machine is very up-to-date. I'm not saying we need it every single time, my problem is that I dont quite understand what's happening. Maybe you can help me:

Inside the process_result callback we do call vim.api.nvim_* functions. There are restrictions on when you can call nvim apis, right? Something like "It is an error to directly invoke vim.api functions in vim.loop callbacks.". I'm not sure in what context is the finder running in (an "libuv/plenary async" thread?) and if from that context I can call vim apis. Without the scheduler it sometimes failed trying to call the vim api.

What is async.util.scheduler() doing? Maybe then I get it. Plenary says "An async function that when awaited will await the scheduler to be able to call the api." Does that mean that my thread is yielding until it can be scheduled on the vim main thread and then I can call vim apis safely? That would mean only right after scheduler() because if something else yields on my thread I might already again be off the main thread?

@dkuettel dkuettel force-pushed the dk/smooth-one-shot branch from c1ef522 to 3cfec52 Compare May 1, 2022 16:11
@dkuettel
Copy link
Author

Sadly, this is still an issue :) I tried it with b79cd6c and it doesnt handle slowly produced results gracefully.
Someone mentioned #1491 might fix this instead of my change. Is that still the case, should we wait for that?
However, my change is orthogonal to the rendering approach, isn't it? It changes how we collect results, not how we render them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants