Autocomplete: Bring back completion synthesization from prior requests and retest all inflight requests

[philipp-spiess]

This PR brings back a completion cache that is able to synthesize results even if the request has changed. It's much simpler than the old request cache and only keeps a maximum of 50 completions around.

When the cache is queried, we compare the document position with all cached completions for the same file and if there's something salvageable, we synthesize a new completion base off of the previous one.

The data structures are kept much simpler now. Instead of having a map of multiple arrays of completions, we flattened the data structure to only be an array of completions. Inflight request logging was also changed from a per-file map to a flat set.

Todo

I noticed what appears to be a regression that I’m still investigating. Sometimes the last candidate response will not be the actual last completion that was visible. You can notice this happening at ~26sec in this video:

Screen.Recording.2023-08-03.at.15.11.40.mov

Test plan

Type a console.log statement, leafing only a tiny pause after one character. Observe that the response is going to be marked as CacheAfterRequestStarted:

[philipp-spiess]

I noticed what appears to be a regression that I’m still investigating. Sometimes the last candidate response will not be the actual last completion that was visible. You can notice this happening at ~26sec in this video:

I think this happens because now, multiple completion requests can be merged into a new completion array but we do not keep track of which exact completion of the array was the last visible one. I think we'll have to add this because we only ever want this logic to trigger if the exact same completion is visible again. I know we need to store all of the returned items so the hover tooltip works, but for the comparison we only need to look at one.

Edit: This seems to work

Screen.Recording.2023-08-03.at.15.30.55.mov

[sqs]

Thank you! I added some comments that IMO should be addressed before merging.

Take my input as that from a random dev out there--you make the final decision here.

I do worry about the network cache becoming too complex and bug-prone as it:

1. Models a subset of document contents, document edit behavior, and editor view state.
2. Interacts with the way that ghost text is and should be shown (ie the LastCandidate "cache"). (My concern here is eliminated if the Opt-out from the last candidate logic when a previously invisible ghost text would be used commit is reverted, as then the LastCandidate cache takes more of a primary/differentiated role then and there is less overlap.)

I also totally understand the value of cache entry synthesis from already in-flight requests that fortunately predicted the user's subsequent typing.

The // glitch (cac-slashslashspace-glitch.mp4 posted below) is an example of what I am worried we will be encountering a lot with the network cache. Maybe it is just that one issue and then it's good! I don't know.

I don't know the right solution or trade-off, and I trust you to make the call.

[philipp-spiess]

@sqs Thank you for the detailed feedback!

I’m now thinking that we can probably just reuse the "last candidate" logic here instead of using a fully blown cache like this. So when a previously in-flight request finishes and we do the "retest cache" logic, we can also pretend it is the last candidate and see if it would still apply.

[philipp-spiess]

Hm, not happy with this approach either:

• The last candidate logic would need to be refactored since it also takes ranges and log IDs into account, something we don't have in the request manager
• Reusing the last candidate logic and only comparing a newly resolved response against all inflight requests does not seem to be enough. While the tests pass and everything works as expected, the real world impact is not as noticeable (especially with the fast StarCoder 7b model I’m running - an exact timing like this does not occur often). So it seems like we also want to synthesis before we even start a request in case a prior request form the same line was already resolved. Something the previous CompletionCache approach supported.
  • I wonder now if this change is the actual culprit that caused the latency regression, the impact of this can be more than the ~10% of synthesized responses for inflight requests. Will try to check if I can find logs to see how often cache hits were used previously and how often they are now.
• Supporting the forward-typing only case seems to be good enough though.
• I have a hunch that we do need to run the post process logic before we reuse the completion to synthesise a new one. This is likely what's happening in Autocomplete: Bring back completion synthesization from prior requests and retest all inflight requests #559 (comment). Not quite sure how to do that yet, I want to avoid making the request manager aware of the post process step.

I'll need think about this a bit more (and also really start writing my reviews now 😐).

[philipp-spiess]

Memo to self: Check what happens if we return a completion result even though a new inline complete call was triggered.

Expectation: VS Code will disregard the completion (unless it matches the new prefix perhaps?)

Idea: If that's the case, we might return instead of cancelling all completion results and only completely rely on the "last candidate" logic. We just need to make sure these are not logged as being visible.

[philipp-spiess]

Ohh this works great!

[philipp-spiess]

Todo

• Add at least one test case to make sure we indeed yield aborted requests so that the last candidate logic can kick in.
• Ensure that latencies are correctly logged now that we not always log events after they are emitted
  • It's best we create an extra callback after the data is post-processed to rule out any issues like this in the future