Performance problems on large repos about neotest HOT 23 CLOSED

I see "Common root not found" errors

As far as I remember this is actually a bit of laziness on my part because I don't open multiple projects in one NeoVim instance. The trees just need to be merged inside of a new parent here https://github.com/rcarriga/neotest/blob/master/lua/neotest/lib/positions/init.lua#L84. The way the new file detection works, it shouldn't (could be wrong) scan the new parent directories.

However I do prefer the idea of having separately maintained trees for each project as it'll present a lot cleaner IMO

from neotest.

Separate to the project config work, I've done some benchmarking and found an easy optimisation within the treesitter internal code. I've implemented it in the the latest master and I'm seeing a 30% reduction in time to parse the cpython repo 😁 It should hopefully improve performance for most languages. This won't solve all performance issues but it will hopefully improve things a bit.

I'm also planning on creating a consumer that can run benchmarks easily so others can see what is affecting performance

from neotest.

No need, I have the setup_project call in .nvimrc and the Neotest config in after/plugin/neotest, so the configuration was being overwritten.

Works great, thank you.

from neotest.

Great to hear! I'm going to merge the PR so feel free to make the change to neotest-elixir, I'll get around to submitting PRs to other adapters soon as well.

from neotest.

The find_files actually completes within ~2 seconds, but then running adapter.is_test_file 250k+ times takes several minutes (vim-test adapter). After that freeze, there is a second freeze while it tries to write all the events to the logfile (hasn't finished yet).

Yep the vim-test test detection is horrifically slow. It should however only trigger if an open buffer registers as a possible test file so if you're not actually using it for that repo I'd suggest disabling it for whatever filetype is causing it to run (this is just a workaround for this particular case)

On to the broader issue

If adapters need to control the search path, their API would have to change from testing individual files that neotest gives them to calling back into neotest with whatever their search config is. IDK if that refactor is worth it, which is why I could go either way on making the search configurable.

I actually originally wrote the adapter interface to work this way but switched to the current way to avoid running multiple crawls but now the initial detection of tests (project root detection, open buffers etc) is smarter.

Another direction could be to customize the search on a per-directory basis instead of per-adapter.

I can see this being a pretty popular request not for performance but as you say for the per-project runner/settings. I think it's inevitable something will need handle this. The proposed interface looks pretty good AFAICT, though the adapters I've written (and I imagine others) would need to be refactored to handle configuring multiple instances. Wouldn't be too much work though.

I think the project/discovery configuration would solve this issue better than having adapters controlling the search as it solves other issues too so for now I'll say let's go with that. This will be a rather large change so it'll take some time to work on. I think I'd like to take a stab at it myself just to get a grasp of the implementation details. I can provide more thoughts on interface as I put some more thought into it.

I'll probably be making more proposals, requests, and questions. Is filing an issue the best way to start a discussion, or would you prefer some other channel?

Love to hear that, thanks! To be honest I haven't thought about it much. I don't mind using issues right now but do feel it could get a bit noisy quickly depending on traffic. I've not used GitHub discussions much so I can take a look at that. Open to other suggestions as well! I'm planning on setting up a neotest org to keep all the repos under so I can add it to the admin TODOs 😅

from neotest.

the adapters I've written (and I imagine others) would need to be refactored to handle configuring multiple instances

Oh this would be excellent! That might solve another issue I've been having with multiple directories open in the same vim. If both dirs use the same test adapter, I see "Common root not found" errors. But if each dir were using a different adapter instance that might be one way around it.

I think the project/discovery configuration would solve this issue better than having adapters controlling the search as it solves other issues too so for now I'll say let's go with that. This will be a rather large change so it'll take some time to work on.

Sounds good! LMK if I can help at any point.

from neotest.

Finally got around to implementing per-project configs 😅 I've got it in #100 just for testing but it seems to work nicely AFAICT. If anyone runs in to issues please let me know!

from neotest.

Awesome!! I'll test it out when I get a chance and leave feedback

from neotest.

I'm also having lags on bigger repos. This is especially annoying during BufAdd and BufLeave events, disabling the test dicovery (#30) in config doesn't solve the issue disabling test discovery does solve the issue 👍🏻

EDIT: I had wrong config setup 🤦🏻 all good.

from neotest.

OK I've added two new config options to help with discovery performance.

1. Concurrent discovery control: You can now specify the number of concurrent workers parsing files. The default is CPU cores + 4 (this is a bit random, just went with what Python's ThreadPoolExecutor defaults to). Set this to 1 and it should reduce CPU usage considerably but it will also slow down test discovery.
2. Directory filtering: The proposed dirs option would probably suit most people but many projects put tests alongside corresponding code files so I thought this was a bit inflexible. Instead there's now a filter_dir option that can filter what directories to search for test files.

In terms of user experience, directory filtering is preferred over limiting concurrency which is preferred over disabling discovery altogether. I believe that these features are sufficient to tackle nearly all performance related issues. Would love to hear feedback on these! 😄

If you are still experiencing performance issues and you've disabled concurrency and set up filtering to at least rule out scanning directories like node_modules or venv please let me know but also be sure to rule out other plugins. I rarely experience significant slowdown due to neotest, it's almost always LSP (wish there was a better Python LSP 😅)

from neotest.

I tried adding the filter_dir option, but it looks like it isn't being called. My project is set up like this:

require("neotest").setup_project(vim.loop.cwd(), {
  adapters = {
    require("neotest-elixir")({ extra_formatters = { "ExUnit.CLIFormatter", "ExUnitNotifier" } }),
  },
  discovery = {
    enabled = true,
    filter_dir = function(name, rel_path, root)
      print(name, rel_path, root)
      return false
    end,
  },
})

I see no messages and no directory is filtered.

Also, in Elixir all tests belong in a test/ folder, it isn't possible to put the test file alongside the source file, and because of the recent change in the find function, basically every project has to add the filter_dir option as Neotest is now detecting test files in the deps directory.

Would be possible to for an adapter to set a default filter_dir function?

from neotest.

Would be possible to for an adapter to set a default filter_dir function?

Yep definitely think this is needed 👍 Adapters can now define a filter_dir function with the same signature

I tried adding the filter_dir option, but it looks like it isn't being called.

Everything looks correct. Can you enable debug logging and submit a separate issue?

from neotest.

I've not heard further reports on performance issues so I'm not sure how prevalent they are anymore but I wanted to have a little fun with using Neovim's RPC API. I've implemented the treesitter parsing to be run in a separate Neovim instance so that the only cost on the main editor instance is deserialising results. It's sitting in a PR right now #119, it'd be great to get some feedback on it.

It should never expose any errors directly to the user, instead logging errors and falling back to the current in-process parsing, so when testing please check the logs (I'd recommend the INFO level). The main issue I'm anticipating is lazy loading, I've added a call for require("neotest") to the subprocess so if you do lazy load, just add module = "neotest" to the config for packer (or equivalent for other managers).

Also some adapters (neotest-elixir, neotest-go, neotest-dotnet, neotest-rspec, neotest-phpunit) use callback functions for the parsing which can't be done over RPC. The solution here is to provide a string to evaluate to a function in the child process (e.g. require("my-adapter")._build_position so if adapter authors or people willing to make the change wish to test, it should be relatively simple.

If testing goes well, it might be possible to expose more helpers to run CPU intensive work in the subprocess

from neotest.

I made the change, and it seems to be working well, although I didn't notice any change in the time it takes to discover.

What I did notice is that discovery is triggered twice.

When I open the summary, it outputs:

Searching <project folder> for test files
Discovering files with 20 workers

And then prints a bunch of Parsing <file> lines.

Then, I can see the Searching and Discovering lines again, and the Parsing line for every file.

Any file I searched in the log appeared twice, and I removed the log between runs.

Is there a reason for that?

from neotest.

Thanks for testing it out!

I don't think that'd be related to these changes as there's no change in the discovery logic, this is purely in the treesitter library functions. Are the second log lines from the parent or child instance (has CHILD in the line)? Also are you doing anything or just letting discovery run and then exiting?

Edit: Also to clarify, this won't speed up discovery (it's doing the same work), it just shouldn't ever block the editor because it's running in a separate neovim process

from neotest.

Yeah, I didn't think it was related to these changes, but it is related to performance.

Are the second log lines from the parent or child instance (has CHILD in the line)?

They are all from the parent instance.

Also are you doing anything or just letting discovery run and then exiting?

I would either open the summary and check the logs, or run a test file and check the logs.

I forgot to mention that the lines Initialising client and Initialisation finished in X seconds would only print once, before the first batch of logs and after the second batch of logs, respectively.

from neotest.

Thanks for the info! Could you provide a clean log for some repo that I can access?

from neotest.

Sure. One more thing I noticed is that the duplicate discovering only happens when I open the summary while having a test file loaded in a buffer. If I don't have a test file open, it only prints each file once.

Here is a repo I use for testing. It has very few test files, so the log is not long.

parquex_info.log
parquex_debug.log

In the log at INFO level it is easier to notice the duplicates, but the DEBUG one provides more info, so I included both.

from neotest.

Ah thanks!

duplicate discovering only happens when I open the summary while having a test file loaded in a buffer

This was the key 😅 It was an existing issue that has been fixed in master

from neotest.

Great! The difference is very noticeable in my work repo.

from neotest.

I'm going to close this issue as all of the originally requested features/changes have been made or have alternatives in place. If there are still performance issues, then we can address them specifically in separate issues.

from neotest.

Thanks for the pull requests on the adapters. It definitely feels faster. ❤️

from neotest.

Fantastic, love to get the feedback 😁

from neotest.