-
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: Enable keepalive agent for Node #868
Conversation
@@ -21,6 +22,8 @@ import { getRgPath } from './rg' | |||
* (Node.js/Electron). | |||
*/ | |||
export function activate(context: vscode.ExtensionContext): ExtensionApi { | |||
initializeNetworkAgent() |
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.
@olafurpg Is this method being run when the agent is starting up? :)
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.
Yep looks like the Agent calls this function: https://sourcegraph.com/github.com/sourcegraph/cody@ps/autocomplete-keepalive/-/blob/agent/src/agent.ts?L22
free wins for agent again 💅
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.
Thanks for the ping. Yes, the agent tries to be as invisible as possible 🙈
export function fetch(input: RequestInfo | URL, init?: RequestInit): Promise<Response> { | ||
return isomorphicFetch(input, { | ||
...init, | ||
agent: agent.current, |
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.
Out of curiosity, what is your |
@umpox Oh, interesting. Yeah I remember that I have had to deal with that before for the old SG VS Code extension. I have set it to |
@umpox Yeah this is not doing anything for the default value. uh noes. I wonder if we can monkeypatch it somehow. |
@umpox yeah i think there's nothing we can do when the default option is set. So this change will only have an impact for users that either have Wow this is frustrating. |
I found a way 😐 |
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.
👍
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.
Tested it locally. Great find, @philipp-spiess!
|
||
import { agent } from './fetch' | ||
|
||
const nodeModules = '_VSCODE_NODE_MODULES' |
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.
This field is marked as deprecated, but we're safe since errors are ignored, and we won't get latency improvements in the worst-case scenario.
/** | ||
* By hard-requiring isomorphic-fetch, we ensure that even in newer Node environments that include | ||
* `fetch` by default, we still use the `node-fetch` polyfill and have access to the networking code | ||
*/ | ||
import isomorphicFetch from 'isomorphic-fetch' |
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.
By networking code, do you mean access to RequestInit['agent']
? If VS Code migrates to the node.js version with native fetch, can we migrate to the new HTTP stack alternative (Dispatcher)?
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.
@valerybugakov Yeah I found that relying on the polyfilled global fetch
, node-fetch
was used in VS Code (older Node.js version) but the native fetch was used on Node 20 (CLI). By requiering fetch here, it would always use node-fetch
in Node environment.s
Wow I wasn't aware of undici this is amazing. I’m glad we see some improvements there. We'll have to still use some polyfills though because this code can also run inside a browser 🦭
Co-authored-by: Valery Bugakov <skymk1@gmail.com>
Co-authored-by: Valery Bugakov <skymk1@gmail.com>
Co-authored-by: Valery Bugakov <skymk1@gmail.com>
We found another small way to improve overall latencies: Making sure we keep the TCP connection alive and reducing the need for subsequent SSL/TLS handshakes.
In Node, reusing TCP connections is not the default behavior (like it is for example on our Go backends). To fix this, we have to make sure we create a custom network agent that is configured to keep connections allive.
Since this is Node only, we are using a mutable ref objects to not run any of those code paths on the web branches.
c.f. https://sourcegraph.slack.com/archives/C05497E9MDW/p1693387804620389
Test plan
Tested using the local completion CLI script
We found about a 5% latency improvement: