perf(childprocess): spawned processes are tracked and monitored. by Hweinstock · Pull Request #6304 · aws/aws-toolkit-vscode

Hweinstock · 2025-01-03T14:38:44Z

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

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.

Future Work

Built on this: (feat(childprocess): add new interfaces for tracking child processes Hweinstock/aws-toolkit-vscode#8)
Integrate telemetry into the tracker instead of only logging.
Add public-facing (non-dev) command so that users can view active processes and their usages.

Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
Your code changes must meet the guidelines in CONTRIBUTING.md.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

Hweinstock · 2025-01-06T18:07:26Z

packages/core/src/shared/utilities/pollingSet.ts

 */

-import { globals } from '..'
+import globals from '../extensionGlobals'


avoids circular dependency.

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.

Is there every a case where importing from index.ts is preferred? Or is this a pretty straight-forward "don't do this" pattern?

PR for lint rule: #6372

Hweinstock · 2025-01-06T18:08:03Z

packages/core/src/shared/utilities/pollingSet.ts

        }
    }
-
+    // TODO(hkobew): Overwrite the add method instead of adding seperate method. If we add item to set, timer should always start.


plan to do as quick follow up, since this involves touching some other files unrelated to process work.

nkomonen-amazon · 2025-01-14T21:59:57Z

packages/core/src/shared/utilities/processUtils.ts

+    }
+
+    public async getUsage(pid: number): Promise<ProcessStats> {
+        const getWindowsUsage = () => {


nit: Instead of const getWindowsUsage... you can do function getWindowsUsage() { ... } and move it to after the logic of getUsage(). I find it cleaner.

justinmk3 · 2025-01-17T12:48:34Z

packages/core/src/dev/activation.ts

    | 'editAuthConnections'
    | 'notificationsSend'
    | 'forceIdeCrash'
+    | 'startChildProcess'


Cool, I love that we keep enhancing dev-mode.

justinmk3 · 2025-01-17T12:55:29Z

packages/core/src/shared/utilities/processUtils.ts

+            // isWin() leads to circular dependency.
+            return process.platform === 'win32' ? getWindowsUsage() : getUnixUsage()
+        } catch (e) {
+            ChildProcessTracker.logger.warn(`Failed to get process stats for ${pid}: ${e}`)


If it ever fails we should probably not attempt on future invocations?

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?

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.

justinmk3

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 About command, but that's too limited.

Hweinstock · 2025-01-17T19:14:29Z

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.

Hweinstock added 11 commits December 23, 2024 15:16

implement basic tracker class

53aaeef

add option in dev menu to start processes

d48f1b3

add dependency to track pid usage

44fe332

warn on exceeding thresholds

9d40215

add first test case

bcb6423

expand test suite

8975c33

simplify and expand test suite

72681fd

add more tests

5c652b6

refactor

6bb8c74

remove tracker interface

a9224d3

refactor wording

4bfff0a

Hweinstock changed the title ~~perf(ChildProcess): spawned processes are tracked and monitored.~~ perf(childprocess): spawned processes are tracked and monitored. Jan 3, 2025

Hweinstock closed this Jan 3, 2025

Hweinstock reopened this Jan 3, 2025

Hweinstock added 9 commits January 3, 2025 09:49

Merge branch 'master' into process/track

61e6fbf

use topic logger

2df7506

improve test performance by properly stopping timer

efe336e

fix circular dep

30ab5b5

add debugging messages for flaky test

10537e1

add safe wrapper around stop

46a556b

avoid waitUntil with stubbed clock

3c1531f

remove dependency

e9b4605

avoid circular dep

c1975f9

Hweinstock force-pushed the process/track branch from e69eeb7 to 601050d Compare January 3, 2025 16:30

fix logging statements

59f8215

Hweinstock force-pushed the process/track branch from 601050d to 59f8215 Compare January 3, 2025 16:33

Hweinstock added 4 commits January 3, 2025 11:38

clean up tests

39175c0

increase waiting period to avoid flakiness

e04eefe

add some debugging information for flaky test

abf3603

handle result and process individually and explicitly

4f2eaf0

Hweinstock added 2 commits January 6, 2025 11:48

remove repeat tests

b0976b3

reword logging statement

ec3abbf

Hweinstock force-pushed the process/track branch from 3853ce8 to ec3abbf Compare January 6, 2025 17:43

Hweinstock marked this pull request as ready for review January 6, 2025 17:58

Hweinstock requested a review from a team as a code owner January 6, 2025 17:58

Merge branch 'master' into process/track

422fa1d

Hweinstock commented Jan 6, 2025

View reviewed changes

Hweinstock mentioned this pull request Jan 7, 2025

feat(childprocess): add new interfaces for tracking child processes Hweinstock/aws-toolkit-vscode#8

Closed

nkomonen-amazon reviewed Jan 14, 2025

View reviewed changes

justinmk3 mentioned this pull request Jan 14, 2025

refactor(amazonq): Improve robustness of lsp installation process #6364

Merged

Hweinstock added 3 commits January 15, 2025 09:51

refactor sub-functions

7a702ff

Merge branch 'master' into process/track

db973f5

merge in upstream changes

2e94873

justinmk3 reviewed Jan 17, 2025

View reviewed changes

justinmk3 approved these changes Jan 17, 2025

View reviewed changes

justinmk3 merged commit d55b8e1 into aws:master Jan 17, 2025
20 checks passed

Hweinstock mentioned this pull request Jan 17, 2025

feat(childprocess): add new interfaces for tracking child processes #6394

Open

Conversation

Hweinstock commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Future Work

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hweinstock commented Jan 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Hweinstock commented Jan 3, 2025 •

edited

Loading