more stable autocomplete

[sqs]

Makes autocomplete not jitter as much as you type, with a cache and a few other techniques to help increase the (1) cache hit rate and (2) VS Code's ability to reuse and not invalidate existing completion results.

Why this change?

• Extract the non-VS Code-specific parts of getInlineCompletions so we can separately test it and the InlineCompletionItemProvider logic.
  • All test cases were ported to the new getInlineCompletions (and all pass without modification in substance).
  • This also helps with extracting completions to the agent.
• Make the cache and RequestManager simpler and dumber by exploiting VS Code's existing behavior (of showing w/o jitter a full-line inline completion while it is refreshed in the background).
  • Remove the existing cache entirely and replace it with a simpler cache that just memoizes calls (inside RequestManager) and does not have any other text logic.
  • Make the RequestManager not need to support logId rewriting or retesting caches for a document (see below).
• Separately handle the case of "typing the last suggestion" (e.g., typing as suggested by the ghost text) vs. the general case of cached network requests. This case is the only one where we want to reuse the previous logId, and we can do so simply instead of (if the 2 cases are conflated) requiring the cache to support logId rewriting.

Examples of bugs or undesirable behaviors this fixes:

• Absolute minimum jitter when deleting or typing ghost text.
• As long as you are still typing ghost text exactly as suggested, it won't replace it underneath you with a new completion.
• If you delete through ghost text past the initial trigger point, it will always suggest something new.
• When you press delete on an empty but indented line with ghost text, the ghost text remains in place (no jitter) until you type something different.

Test plan

Use completions and watch the logIds. Try typing or deleting over ghost text.

[philipp-spiess]

Nice! I like the changes. Some comments inline. I think with this approach, the LRUCache map doesn't really make sense as a data-structure. I think the way we use it, we also don't count when a cached completion is retrieved so this behaves more like a fifo queue now where an array might be enough?

The changes look good though. I would argue that it's not simpler though, there's more custom behavior that the cache now implements (previously it conceptually was just a map). However, the implementation is easier - the map lookup was definitely a premature optimization from my side!

[sqs]

@philipp-spiess Thank you for the comments! All great points. And after sleeping on it, I see a bunch more places where it needs work. Will keep this in draft and keep working on it.

[sqs]

@philipp-spiess I think we can significantly simplify the cache from what I have here and use VS Code's builtin "reuse" logic more. Stay tuned...

[sqs]

Note: This PR is based on the following other PRs that will be merged independently (then this PR branch will be rebased):

• try to show completion when suggest widget is showing #478
• do not swallow provideInlineCompletionItems exception #480
• return completions from the start of current line to reduce VS Code UI jitter #482

[philipp-spiess]

Separately handle the case of "typing the last suggestion" (e.g., typing as suggested by the ghost text) vs. the general case of cached network requests. This case is the only one where we want to reuse the previous logId, and we can do so simply instead of (if the 2 cases are conflated) requiring the cache to support logId rewriting.

This is great, thanks. I was chatting with @vovakulikov yesterday and came to the same conclusion that these are two different scenarios and wanted to post a PR today to do the same 😅

[philipp-spiess]

@sqs This feels very good to use now, wow!

I noticed one case where it's a bit funky still. If you see ghost text and press space, the previous ghost text will stay visible but we do fire off a new request that will eventually replace the visible text, causing some UI churn:

Screen.Recording.2023-08-01.at.11.40.03.mov

[philipp-spiess]

This is a great clean up! I left two inline comments + the main comment in the PR about the remaining "jitter" case (the latter is minor and I’m fine with fixing it later)

[philipp-spiess]

Did you remove this logic on purpose? Looking at the logs, it does trigger on almost ~10% of requests: https://sourcegraph.looker.com/x/PEfxGviZvYk8tmvTFGIDVr

[philipp-spiess]

Thinking about this more, I guess the issue is that the new network caching is no longer able to synthesise completions. Hmm.

[sqs]

Yes, I did remove it for that reason. I did not realize it triggered in ~10% of requests. That is substantial. Do you think this retest-cache behavior is important to preserve for perf? I am not confident in my intuition about the impact from reading the code just now.

[philipp-spiess]

@sqs My intuition is yes. It was also the main motivation for not cancelling inflight requests when a new completion is triggered.

I’m less confident on wether it had a big impact on the overall metrics (something had in the period this rolled out, but we tweaked a bunch of different things) however I could definitely notice the change in my IDE, especially when typing because sometimes you just get immediate completion results and it feels magical.

[philipp-spiess]

I'll keep an eye on the metrics and if the latency regresses we know what to do 👍

[sqs]

@philipp-spiess Thanks! I will address the remaining things and get this merged.

[sqs]

@philipp-spiess Thanks! I just fixed the issue you raised at #442 (comment) and added a lot more tests for the typing-as-suggested-by-last-candidate case.