-
Notifications
You must be signed in to change notification settings - Fork 213
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
Autocomplete: Various latency related tweaks and new eager cancellation experiment #3096
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import { partition } from 'lodash' | |
import { LRUCache } from 'lru-cache' | ||
import type * as vscode from 'vscode' | ||
|
||
import { isDefined, wrapInActiveSpan } from '@sourcegraph/cody-shared' | ||
import { FeatureFlag, isDefined, wrapInActiveSpan } from '@sourcegraph/cody-shared' | ||
|
||
import { addAutocompleteDebugEvent } from '../services/open-telemetry/debug-utils' | ||
|
||
|
@@ -23,6 +23,8 @@ import type { ContextSnippet } from './types' | |
import { lines, removeIndentation } from './text-processing' | ||
import { logDebug } from '../log' | ||
import { isLocalCompletionsProvider } from './providers/experimental-ollama' | ||
import { completionProviderConfig } from './completion-provider-config' | ||
import { forkSignal } from './utils' | ||
|
||
export interface RequestParams { | ||
/** The request's document */ | ||
|
@@ -72,6 +74,9 @@ export class RequestManager { | |
private latestRequestParams: null | RequestsManagerParams = null | ||
|
||
public async request(params: RequestsManagerParams): Promise<RequestManagerResult> { | ||
const eagerCancellation = completionProviderConfig.getPrefetchedFlag( | ||
FeatureFlag.CodyAutocompleteEagerCancellation | ||
) | ||
this.latestRequestParams = params | ||
|
||
const { requestParams, provider, context, isCacheEnabled, tracer } = params | ||
|
@@ -89,7 +94,10 @@ export class RequestManager { | |
// When request recycling is enabled, we do not pass the original abort signal forward as to | ||
// not interrupt requests that are no longer relevant. Instead, we let all previous requests | ||
// complete and try to see if their results can be reused for other inflight requests. | ||
const abortController: AbortController = new AbortController() | ||
const abortController: AbortController = | ||
eagerCancellation && params.requestParams.abortSignal | ||
? forkSignal(params.requestParams.abortSignal) | ||
: new AbortController() | ||
|
||
const request = new InflightRequest(requestParams, abortController) | ||
this.inflightRequests.add(request) | ||
|
@@ -135,7 +143,13 @@ export class RequestManager { | |
}) | ||
|
||
request.lastCompletions = processedCompletions | ||
this.testIfResultCanBeRecycledForInflightRequests(request, processedCompletions) | ||
|
||
if (!eagerCancellation) { | ||
this.testIfResultCanBeRecycledForInflightRequests( | ||
Comment on lines
+147
to
+148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we keep this functionality when This way, we preserve the nice UX where the completion is generated early, and a user continues typing as suggested AND decrease the tail completion delay by 65ms. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m not sure I understand this right. If we want to keep this, we would have to keep more than just the last request alive (so that another request can actually answer another one). Are you recommending we keep all but the last 2 completions active? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed on the call. We're going to follow-up on that in a separate PR. |
||
request, | ||
processedCompletions | ||
) | ||
} | ||
} | ||
|
||
// Save hot streak completions for later use. | ||
|
@@ -154,7 +168,9 @@ export class RequestManager { | |
) | ||
} | ||
|
||
this.cancelIrrelevantRequests() | ||
if (!eagerCancellation) { | ||
this.cancelIrrelevantRequests() | ||
} | ||
} | ||
} catch (error) { | ||
request.reject(error as Error) | ||
|
@@ -163,7 +179,9 @@ export class RequestManager { | |
} | ||
} | ||
|
||
this.cancelIrrelevantRequests() | ||
if (!eagerCancellation) { | ||
this.cancelIrrelevantRequests() | ||
} | ||
|
||
void wrapInActiveSpan('autocomplete.generate', generateCompletions) | ||
return request.promise | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 ☄️