Repository navigation
perf: add latency benchmarks, and fix the latency and correctness issues of a code review - #86
Merged
Merged
Conversation
Nothing measured the cost of cmp-git while the user types or the time until the items show, so a change could make either worse without a sign. benchmarks/ has three groups of cases. keystroke/ measures the checks that nvim-cmp and blink.cmp make on each keystroke, and requests that cmp-git declines. request/ measures routing, parsing, and item formatting. e2e/ runs requests from start to the final callback in a git repository with 1000 commits, with a fake gh, so that no case uses the network. Each case reports its time and the processes that it starts. Sync cases also report the Lua memory that they allocate, and e2e cases report their main-thread time and longest main-thread block. make bench runs the cases. make bench-compare REF=<rev> runs the cases of the working tree on the lua/ code of <rev> and of the working tree, and reports the regressions. The directory is not named bench/, because common global gitignore files ignore that name.
The Benchmarks job runs scripts/bench-compare.sh on each pull request against the first parent of the merge commit, three runs on each side, and appends the report to the job summary. Shared runners are noisy, so the job does not block the merge.
The remote URL parser removed ".git" anywhere in the URL, so the repo of https://github.com/user/user.github.io.git was "userhub.io". For https://gitlab.com/group/sub/repo, the host was "gitlab.com/group", and for ssh://git@host:2222/group/sub/repo, the owner was "sub". Credentials in an https URL stayed in the host, and a trailing slash stayed in the repo. The parser now reads the scheme, the authority, and the path. The host has no user info. An https or http remote keeps its port, because the API is on the same origin. Other remotes, such as ssh:// remotes, drop the port. The owner is all path segments before the last one, so a GitLab subgroup gives the owner "group/sub", as the scp-like form already did. The repo is the last segment without a trailing ".git" or "/". Local paths and file:// URLs give no repository.
The alias was put in a Lua pattern, with only "-" and "." escaped. An alias such as "gh+work" never matched its host, and a "%" in the replacement host was read as a capture reference. The host is now looked up in ssh_aliases, so that each alias and host is used as it is written.
A buffer of a new file can be in a directory that does not exist yet. get_cwd() returned that directory, and vim.system throws ENOENT for a cwd that does not exist, so the error escaped from the completion request. get_cwd() now returns the buffer directory only when it exists, and Neovim's cwd in the other cases.
Each completion request started 3 to 5 processes before it could use its cache: "sh -c git rev-parse" in is_git_repo, "sh -c git config" for each remote in turn, and all of it again in Neovim's cwd when nothing was found. A warm commits request took 14.6 ms, and about 4 ms of it was main-thread time for the process starts. Discovery now runs git without sh, and needs two processes: one git rev-parse that tells whether the directory is in a repository and where its config is, and one git config --get-regexp for the URLs of all remotes. With enableRemoteUrlRewrites, git remote -v gives the rewritten URLs of all remotes in one process. The remote order of the remotes option still decides which URL is used. The result is kept for each buffer, so a repeated request starts no process, and concurrent requests share one discovery. The buffer directory and, when the repository was not found there, Neovim's cwd are part of the key, so a renamed buffer or a :cd gives a new discovery. A change to the config file of the repository, for example a new remote, also gives a new discovery. A directory outside a repository is checked again after 5 seconds, because git init in any parent directory can change it. The entry of a buffer is dropped when the buffer is wiped. Octo buffers get their repository from the buffer name and start no process. Discovery now reads the remotes of the repository that it found. When that repository had no matching remote, it used the remotes of the repository in Neovim's cwd, which is not the repository of the buffer.
When the buffer directory was not in a repository, is_git_repo and discover found the repository in Neovim's cwd. But get_cwd() still returned the buffer directory, and command.lua runs every git, gh, glab, and curl command there. Discovery succeeded, and then git log failed outside the repository. get_cwd() now returns the directory where discovery found the repository of the buffer, while the buffer directory and Neovim's cwd are the same as at discovery. In the other cases, it returns the buffer directory if it exists, or else Neovim's cwd, as before.
When vim.system could not start a command, for example because the buffer directory no longer exists, it threw from the completion request, and the request never answered. The command now catches the error and reports a failed command. It also no longer adds PATH to the environment of the command: vim.system adds env to the inherited environment, so the comment that said otherwise was wrong.
The curl fallback put the Authorization header with GITHUB_API_TOKEN or GITLAB_TOKEN in its arguments, so other local users could read the token in the process list. curl now reads the header from stdin with -H @- (curl 7.55 and later). A command spec has a new optional stdin field for this.
When gh and curl (or glab and curl) failed, or when neither was installed, the request never called back. The fallback hid the failure of its last command, and a missing command gave no job. blink.cmp then held back the items of all sources for 2 s, and nvim-cmp waited up to 500 ms. The fallback now reports the failure of the last command that runs. build_list calls back on a failure with an empty list and success set to false. The issue and change request capabilities give one empty list when the commands fail or cannot run, and do not cache it, so the next request tries again.
curl -s exits with 0 on HTTP errors such as 401, 403, 404, and 422. The error object then parsed to no items, and the empty list stayed in the cache for the life of the buffer, for example after the hourly request limit of unauthenticated GitHub. curl now runs with --fail (and -S, so that the debug log shows the HTTP error), so an HTTP error is a failed command. The mention preflight keeps the error body, because it reads the status from it. --fail-with-body is not used, because it needs curl 7.76. A parsed result that is not a list after handle_parsed, such as an error object or null, is now a failure, not an empty list. A null response no longer throws in the mention handlers.
The mention cache got an entry that was in progress before the fetch started, and the entry stayed after a cancel or a failure. Then each later @ in the buffer got an empty, incomplete list, and nothing was fetched again. blink.cmp cancels the request of a list that it no longer shows, so this happened in normal use. Requests that arrive during a fetch now share it: they get the users so far, then the final list. The fetch stops only when all of its requests are cancelled, and then the entry is removed. A failure also removes the entry and gives one final list, with the users of the pages before it, so the next @ tries again. When no command can run, the request gives an empty list. Paging also changes. A page with fewer users than the page size is the last one, so @ no longer waits for an empty second page when the repository has fewer users than limit. The page size stays the same on each page, and the users are cut to limit. Before, limit = 150 asked for per_page=50&page=2, which gave users 51 to 100 again.
The # request fetched the pull requests only after the issues, so a cold # waited for two network calls in a row. Both requests now start at once, and the callback runs when both finish, with the issues first and then the pull requests, as before. When one request fails, the list has the items of the other.
A GitLab merge request with a null description showed the text "vim.NIL" in its documentation, because JSON null decodes to vim.NIL. Only the issue description handled it. The adapters now replace a null or missing title, body, description, and GitLab mention name with an empty string for each item.
Each GitHub # request ran gh repo set-default --view with vim.fn.system before it checked the cache. With gh, this blocked Neovim for about 60 ms on each #, also when the items were cached. Without gh, each # threw E475, although gh is optional. The lookup ran in the cwd of Neovim, not of the buffer, it replaced the repository of octo buffers, and @ did not use it, so @ and # could ask different repositories. The lookup now runs with vim.system in the directory of the buffer, only when a cache misses and gh is installed, and at most once for each repository. Requests that arrive during the lookup wait for the same result. Issues, pull requests, and mentions all use it, and octo buffers keep their repository. The git info of the caller no longer changes.
The lookbehind in the trailer value pattern made Vim use its backtracking regex engine, and the time of a match grew with the square of the line length: 13 ms at 1000 characters and about 300 ms at 5000. nvim-cmp matches the keyword pattern on each keystroke, so typing in a long line, such as a Markdown paragraph in an octo buffer, became slow. The value pattern now anchors the key and starts the value with `\zs`. A new `keyword_pattern` field of a pattern entry gives the keyword pattern a separate form that starts at the colon in `Key:` and `Key:value`, so the keyword offsets do not change. Entries that still have the old pattern get the new patterns.
Routing matched the regex of each pattern action before its cheap `available` check, so a long line in a buffer without trailers, such as an octo buffer, still paid for the trailer regexes. The two trailer value actions matched the same regex twice, and a request routed twice, once before the repository check and once after discovery. Routing now runs `available` first, matches each distinct regex once, and a request keeps the routes that it found first. Entries with the same pattern share one compiled regex.
Core never called back when the directory was not a git repository, or when every matched action declined the request, for example `#`, `@`, or `!` with a Gitea or Bitbucket remote, or with no remote. An unknown action name had the same result. blink.cmp then held back the items of all sources until its timeout of 2 s, and nvim-cmp waited when cmp-git was its first source. Core now sends an empty final list in these cases, unless the request was cancelled.
Each request ran `git config` for each configured remote before it ran the actions, but the commit and trailer actions read only the local repository. A request for them started up to two processes that it did not need. Core now discovers the remote only when an action that can run needs it. The git actions always handle the request, so the actions after them do not count. Legacy `action` entries still get the remote.
Octo buffers name their repository in the buffer name, and discovery reads it from there, but core first required a local git repository. Issues, pull requests, and mentions did not complete in an octo buffer that was opened outside a repository. Core now skips the repository check in octo buffers when an action that can run needs the remote. The commit and trailer actions still need a local repository.
The issue, pull request, and mention capabilities give the same cached item tables to each request, and core wrote the text edit into them. blink.cmp writes `pos` and `score_offset` into the items that it gets. On a later request at another column, blink.cmp moved the edit by the old position and replaced the wrong range, and a configured score offset grew with each request. Core now gives each request shallow copies of the items with a new text edit. The items of one response share one range, so a request costs no more time than before and allocates less.
nvim-cmp gives the cursor as a UTF-16 index in `cursor.character`, and it reads the text edit ranges as UTF-16 unless the source names another encoding. cmp-git found the start of the edit in bytes, so a multibyte character before the cursor moved the edit: `für #12` became `für ##123`, and `héllo @ab` became `héllo @@login`. The nvim-cmp source now reports `utf-8` in `get_position_encoding_kind`, and takes the cursor from the byte column `cursor.col`. The blink.cmp path already used bytes.
When the user typed after an incomplete list, such as the first page of mentions, nvim-cmp asked again with the trigger kind TriggerForIncompleteCompletions. cmp-git read no trigger character for that kind, so the request got an empty, complete answer, and the list stopped updating while the mentions loaded. The source now reads the trigger character from the line for this kind, as it does for Invoked requests.
Core put the trigger characters into a Vim regex collection, and the nvim-cmp and blink.cmp adapters put them into Lua pattern sets, without escaping. A trigger character such as `]`, `^`, or `-` changed the meaning of the pattern or made it invalid, and `%` made the Lua pattern throw an error. Core now escapes the characters for each pattern language, and both adapters find the trigger character with the same function. A config without trigger characters and patterns gets no keyword pattern, in place of an invalid one, so nvim-cmp uses its default.
blink.cmp adds the items of each response of a request to the items of the earlier responses. A request that called back more than once, such as mentions that load a first page and then the full list, showed each item of the first page twice. The blink.cmp adapter now forwards only the first response of a request. blink.cmp keeps `is_incomplete_forward` of the first response, so an incomplete first page makes it ask again on the next keystroke, and that request gets the cached items. blink.cmp cancels the earlier request when it asks again, so a request that has answered is not cancelled: it continues to fill the cache.
Core called `repository.is_git_repo` and `repository.discover` without a buffer, so they read the buffer that was current when they ran. Discovery waits for git, and the user can switch buffers in that time, so a request could get the repository and remotes of another buffer. Core now passes the buffer of the request to both functions.
utils.trim used the pattern ^%s*(.-)%s*$, which tests the end pattern at each position of a whitespace run. A run of 8000 spaces in the middle of a string took 183 ms, and a run of 32000 spaces took 2.9 s. The commit parser trims each subject and body, so one such commit blocked the editor. trim now finds the first and last non-space bytes with two loops, so it reads each byte at most once. The simpler pattern ^%s*(.*%S) is not used, because it is also quadratic when the string has only whitespace. The new utils.trim_range gives the same bounds for a range of a string, so that a parser can trim a field without a copy of it.
The first trailer identities request parsed 1000 commits in one block of 8 ms on the main thread, and allocated 954 KB. The values parse took 3.4 ms and 1 MB, and the commit parse took 7.3 ms. Each parser split the output into a table of records, split each record with vim.split, and trimmed each field with a pattern. The pattern (.-)%s*$ on the trailer values was also quadratic on long runs of spaces. The parsers now find the NUL and unit separator bytes with plain string.find and read each field in place, so that they make no table or string for each record. The identities parse skips a trailer value or an email that it saw before, because a repeat from the log changes nothing, and parses an identity with plain searches for < and >. A key is lowered once for each casing. The output does not change: a test compares the new parsers with copies of the old ones on generated logs with empty fields, spaces, CRLF, folded values, keys in different case, and duplicate emails. With the benchmark fixtures: identities_parse_1000 takes 0.6 ms and 73 KB, values_parse_1000 0.7 ms and 144 KB, commits parse_1000 0.5 ms and 270 KB, and keys_parse_100 34 us and 3 KB.
In a repository without commits, git log fails, and the commit request returned without a callback, so : never answered. Without git, each git capability got no job and gave no callback. blink.cmp then held back the items of all sources until its 2 s timeout, and nvim-cmp waited up to 500 ms. Each commit and trailer request now ends with one final list. A failed commit or values log gives an empty list. A failed keys log still gives the built-in keys, and a failed identities log or git var still gives the result of the other command. A failure is not cached, so the next request runs git again. Before, the keys, identities, and values caches kept the result of a failed log as if it were the history.
The identities request started git var GIT_AUTHOR_IDENT only after git log had finished, so the first list waited for both processes, one after the other. The request now starts both commands at once and gives the list when both have answered. Cancel stops both. A failure of either command still gives the result of the other, and is not cached.
Core asks the trailer keys, identities, and values whether they are available on each request. Each kind read the filetype through `vim.bo[bufnr]`, which makes a new table on each read, before the checks of the line. A request on a line of prose read the filetype three times, and the trailer checks were most of the allocation of a declined request. The checks of the cursor row and of the line now come first, so a line without a trailer key skips the buffer for identities and values. The filetype is read with `nvim_get_option_value`. A declined nvim-cmp request allocates 1.0 KB in place of 2.6 KB, and takes 1.2 us in place of 4.3 us.
Discovery now reads the buffer of the request, but the sources and the commands still read the current buffer. Core calls the sources after discovery, and a command starts after its source, so a buffer switch in that time ran `git log` and `gh repo set-default --view` in the repository of another buffer. The GitHub and GitLab sources also cached the items under the other buffer. Core now passes the buffer of the request to the hosted sources, and the sources and the git capabilities pass it to `command.build` in the new `CommandSpec.bufnr`. A legacy callback action calls a source without a buffer, so the current buffer stays the default. The buffer can be wiped while the request waits. `is_git_repo` and `discover` now answer at once for an invalid buffer, without a repository, and `get_cwd` gives the working directory of Neovim. Before, they threw an error.
- Discovery uses the working directory of Neovim when the file is not in a git repository, keeps its result for each buffer, and reads it again when the git config changes. - The gh default repository also applies to mentions, is read once for each repository, and is not used in octo buffers. - Failed commands and HTTP errors are not cached. - Pattern entries can set `keyword_pattern`, a lookbehind is slow in a keyword pattern, and the old trailer value pattern is upgraded. - curl must be 7.55 or later, because the token goes on stdin. - The default trailer value entry has the new patterns.
Core required the git, GitHub, and GitLab sources and the repository module at the top of the file, so `setup` loaded all the modules of cmp-git: 388 KB and 2.0 ms. Most sessions use one provider, and a request that cmp-git declines needs no source. Core now makes each source when an action first reads it, and it requires the repository module in the requests that need it. A hosted action checks the host of the remote before it gets the source, so a GitHub remote does not load the GitLab source. The line checks of trailer completion move to `trailer_lines`, so that a declined request does not load the trailer log parsers. `setup` now loads 163 KB in 0.8 ms. A test runs `setup` and a declined request in another Neovim and checks the modules that they load.
`startup/require` measures only the modules that load with the nvim-cmp and blink.cmp adapters. The other modules now load on the first request that needs them, so their size no longer shows in a benchmark. `startup/setup` measures the modules and the new source of `setup`, and `startup/require_all` loads every module. CONTRIBUTING explains how to keep a module out of the setup.
The owner of a remote is all path segments before the last one, so that GitLab subgroups work. A GitLab instance can also be under a path prefix, for example `https://example.com/gitlab`. For the remote `https://example.com/gitlab/group/repo.git`, cmp-git used the host `example.com` and the owner `gitlab/group`. The API of this instance is at `https://example.com/gitlab/api/v4`, and the project is `group/repo`, so each GitLab request failed. An entry of `github.hosts` or `gitlab.hosts` can now have a path prefix, such as `example.com/gitlab`. Core gives the GitHub and GitLab hosts to discovery. After the SSH aliases, discovery moves the prefix of a matching entry from the owner to the host. If more than one entry matches, the longest prefix wins. The path of an SSH remote has no prefix. Discovery then uses the prefixed entry if it is the only one for the host, and if the hosts do not have the plain host. With curl, the GitLab API URL is then `https://example.com/gitlab/api/v4`.
The docs said that a host entry must be the same as the host in the URL of the remote. A GitLab entry can now also have a path prefix. The section about hosted instances now shows an entry with a path prefix and the API URL that curl uses. It also tells how cmp-git selects the entry for an SSH remote. The README config comment for `gitlab.hosts` shows the format.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds benchmarks for typing latency and completion response time, and a CI job that compares each pull request with its base. It then fixes the latency and correctness issues that a review of all the code found. Neovim 0.12 stays the minimum, so the async code uses
vim.systemcallbacks, notvim.async.Benchmarks
benchmarks/has three groups of cases:keystroke/: the checks that nvim-cmp and blink.cmp make on each keystroke, and declined requests.request/: routing, parsing, item formatting, and the startup cost.e2e/: one request from start to the final callback, with real git commands in a repository of 1000 commits, and a fakegh.make benchruns the cases.make bench-compare REF=<rev>runs them on thelua/code of two revisions and reports the regressions.BenchmarksCI job runs the comparison on pull requests and writes the report to the job summary. Shared runners are noisy, so the job does not fail the check.Results
Compared with
28b959e(the benchmark commit, which has thelua/code ofmain). Each side uses the best of 2 runs.setupThe
BenchmarksCI job reports one regression,startup/require_all: 329 KB to 392 KB (+19%), and 1.39 ms to 1.69 ms. This case loads every module, so it measures the size of the code. The growth comes from new code. Comments do not change the allocation. The modules that grew most:repositorysources.githubtrailersandtrailer_linesmentionsconfigsetuploads 171 KB in place of 336 KB. Each other module loads once, in the first request that needs it.Changes
Each change is a separate commit with tests.
Typing latency
499f821: the trailer value pattern used a lookbehind, so Vim used its backtracking engine, and the time grew with the square of the line length. The pattern now uses\zs.normalizeupgrades the old pattern in a user config. A pattern entry can setkeyword_patternto give nvim-cmp another keyword pattern.b7d9c88,1049ac5: the cheap checks run first. Availability runs before the regex, and the line check runs before the buffer reads.113c8e1: a request that no action handles gets an empty final list at once. Before, it got no answer, and blink.cmp held back the items of all sources until its 2 s timeout.Response time
2147e6b: discovery runs once for each buffer, with two git processes and nosh. A change to the git config file starts a new discovery.6b0d4f3: commits and trailers skip remote discovery.5bcb338:gh repo set-default --viewruns asynchronously, once for each repository. Before, it ran withvim.fn.systemon each GitHub request and blocked Neovim for about 60 ms.8d74f8f: GitHub issues and pull requests are requested at the same time.00488f7:git logandgit varrun at the same time for trailer identities.3cfd7e8,2aadd15: the log parsers andutils.trimtake linear time.ef7da7b: the sources and the repository module load on first use. A GitHub remote does not load the GitLab code, and a declined request loads no source.Correctness
04c0730,f874bd1,452adea,113c8e1. blink.cmp gets only the first response of a request:b786654.abc5596), failed commands (f874bd1,452adea), and mentions after a cancel or a failure (1309965). Mentions now page up tolimit.c8d0088,1f1d03b. Before, a buffer switch while git ran used the repository of the other buffer. A wiped buffer gets an empty answer.eadeffd: the nvim-cmp text edits use byte columns, and the source reportsutf-8. Before, a multibyte character before the cursor moved the edit:für #12becamefür ##123.904532f: each request gets new item tables. blink.cmp writesposandscore_offsetinto the items, and it changed the cached items.d18665a: requests with TriggerForIncompleteCompletions find the trigger character, so the mention list updates while it loads.dbf3254: the trigger characters are escaped in Vim and Lua patterns.ceb5c11: issues, pull requests, and mentions complete in octo buffers outside a git repository.4e81ece: JSON null titles and descriptions show as empty text, notvim.NIL.Remotes and repositories
0bf8c63: the remote URL parser handles GitLab subgroups, ports, and credentials.356936e: a GitLab host entry can have a path prefix, such asexample.com/gitlab.338cb0e:ssh_aliasescompares hosts as plain strings, not as Lua patterns.41a1846,a655d00: the commands run in the directory where discovery found the repository. A buffer in a directory that does not exist uses the working directory of Neovim.Security
bf97816: curl gets the API token on stdin (-H @-), so the token is not in the process list.Behavior changes
git logfails. Failures are not cached, so each new trailer value completion runs it again.Docs
docs/configuration.mddescribes discovery and its cache, the gh default repository, failures that are not cached,keyword_pattern, and GitLab hosts with a path prefix.README.mdlists curl 7.55 and has the new trailer value patterns in the default config.CONTRIBUTING.mddescribes the benchmarks and how to keep a module out ofsetup.Validation (local)
make ciexits 0 with Neovim v0.13.0-nightly+1cfebe9 fromPATH(208 tests).make testpasses in thenvim-0_12_0shell.make bench-compare REF=28b959egives the results above.