perf(childprocess): spawned processes are tracked and monitored. #6304
Conversation
Hweinstock
changed the title
Hweinstock
force-pushed
the
process/track
branch
from
e69eeb7 to
601050d
Compare
Hweinstock
force-pushed
the
process/track
branch
from
601050d to
59f8215
Compare
Hweinstock
force-pushed
the
process/track
branch
from
3853ce8 to
ec3abbf
Compare
| @@ -3,7 +3,7 @@ | |||
| * SPDX-License-Identifier: Apache-2.0 | |||
| */ | |||
|
|
|||
| import { globals } from '..' | |||
| import globals from '../extensionGlobals' | |||
There was a problem hiding this comment.
avoids circular dependency.
There was a problem hiding this comment.
We will probably want to add a lint warning.
But for context what is happening is that .. implicitly imports the index.ts module instead of the intended one and that causes the circular dependency issue.
There was a problem hiding this comment.
Is there every a case where importing from index.ts is preferred? Or is this a pretty straight-forward "don't do this" pattern?
There was a problem hiding this comment.
PR for lint rule: #6372
| @@ -44,9 +44,14 @@ export class PollingSet<T> extends Set<T> { | |||
| this.clearTimer() | |||
| } | |||
| } | |||
|
|
|||
| // TODO(hkobew): Overwrite the add method instead of adding seperate method. If we add item to set, timer should always start. | |||
There was a problem hiding this comment.
plan to do as quick follow up, since this involves touching some other files unrelated to process work.
| } | ||
|
|
||
| public async getUsage(pid: number): Promise<ProcessStats> { | ||
| const getWindowsUsage = () => { |
There was a problem hiding this comment.
nit: Instead of const getWindowsUsage... you can do function getWindowsUsage() { ... } and move it to after the logic of getUsage(). I find it cleaner.
| @@ -44,6 +45,7 @@ export type DevFunction = | |||
| | 'editAuthConnections' | |||
| | 'notificationsSend' | |||
| | 'forceIdeCrash' | |||
| | 'startChildProcess' | |||
There was a problem hiding this comment.
Cool, I love that we keep enhancing dev-mode.
| // isWin() leads to circular dependency. | ||
| return process.platform === 'win32' ? getWindowsUsage() : getUnixUsage() | ||
| } catch (e) { | ||
| ChildProcessTracker.logger.warn(`Failed to get process stats for ${pid}: ${e}`) |
There was a problem hiding this comment.
If it ever fails we should probably not attempt on future invocations?
There was a problem hiding this comment.
Not really sure what could cause it to fail here. Do you think there is a risk in continually retrying? Maybe potential log spam? Could this be combatted by setting the polling interval longer?
There was a problem hiding this comment.
Not really sure what could cause it to fail here.
Permissions issues usually.
Maybe potential log spam?
Yeah that's at least my first thought. When a system is in a bad state, it can make things worse if logs "cascade" with a deluge of redundant warnings.
There was a problem hiding this comment.
This is a great starting point. Merging it so it can start to "bake".
Followup ideas:
- include perf results in telemetry (do we have an existing metric it can be attached to?)
- dev-mode can show a "report" of process stats
- we need some way for non-dev-mode users to get a report of the Toolkit "health", for troubleshooting and bug reports. this would be part of that. the current answer is the
Aboutcommand, but that's too limited.
|
Follow up to this work: #6394 |
## Problem Toolkit/Q slow performance is consistently reported by customers. Toolkit/Q features can spawn processes, included many long-lived processes such as LSP servers. The cost of these processes is hidden and hard to troubleshoot. Any misbehaving process will not be noticed except in a coarse "Toolkit/Q causes vscode to be slow" manner. ## Solution - Track all running processes in a map-like data structure. - use a `PollingSet` to periodically "monitor" their system usage (%cpu, memory bytes), logging warning messages when it exceeds a threshold. - Add developer command to instantiate spawned processes via `ChildProcess` wrapper to make this easier to test.
Problem
Toolkit/Q slow performance is consistently reported by customers: https://github.com/aws/aws-toolkit-vscode/issues?q=is%3Aopen+is%3Aissue+label%3Aperformance
Toolkit/Q features can spawn processes, included many long-lived processes such as LSP servers. The cost of these processes is hidden and hard to troubleshoot. Any misbehaving process will not be noticed except in a coarse "Toolkit/Q causes vscode to be slow" manner.
Solution
PollingSetto periodically "monitor" their system usage(%cpu, memory bytes), logging warning messages when it exceeds a threshold.ChildProcesswrapper to make this easier to test.Future Work
feature/xbranches will not be squash-merged at release time.