Improve Call Stack performance with many threads, fixes #44248 #44249

rsalvador · 2018-02-23T05:29:45Z

No description provided.

weinand · 2018-02-23T17:41:14Z

@isidorn change looks good and makes sense to me.

isidorn · 2018-02-23T18:51:44Z

@rsalvador Thanks a lot for your PR. I left comments directly in the code, once you address those we could merge this in.
I am assigning this to the march milestone since next week we are wrapping up our february release and are only accepting limited changes.

isidorn · 2018-02-23T18:52:26Z

src/vs/workbench/parts/debug/electron-browser/debugService.ts

+		if (timer) {
+			clearTimeout(timer);
+		}
+		this.displayThreadsTimer.set(session.getId(), setTimeout(() => {


Instead of a timeoue please use a Scheduler.
An example usage can be found here https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/debug/electron-browser/callStackView.ts#L38

isidorn · 2018-02-23T18:56:04Z

src/vs/workbench/parts/debug/electron-browser/debugService.ts

@@ -81,6 +81,7 @@ export class DebugService implements debug.IDebugService {
 	private launchJsonChanged: boolean;
 	private firstSessionStart: boolean;
 	private previousState: debug.State;
+	private displayThreadsTimer: Map<String, number>;


Use lowercase string

isidorn · 2018-02-23T18:56:13Z

src/vs/workbench/parts/debug/electron-browser/debugService.ts

@@ -113,6 +114,7 @@ export class DebugService implements debug.IDebugService {
 		this._onDidCustomEvent = new Emitter<debug.DebugEvent>();
 		this.sessionStates = new Map<string, debug.State>();
 		this.allProcesses = new Map<string, debug.IProcess>();
+		this.displayThreadsTimer = new Map<String, number>();


Use lowercase string

isidorn · 2018-02-23T18:56:34Z

src/vs/workbench/parts/debug/electron-browser/debugService.ts

+			clearTimeout(timer);
+		}
+		this.displayThreadsTimer.set(session.getId(), setTimeout(() => {
+			this.fetchThreads(session).done(undefined, errors.onUnexpectedError);


This will make

isidorn · 2018-02-23T19:03:39Z

@rsalvador Also before we accpet this change I would still need convincing vscode blocks the UI. Since I like what we are already doing - we are refreshing the callstack view with a 50ms delay. So if multiple changes come in that range we will only refresh once. Do you still see the vscode blocking if you change this 50ms to 200 ms for example here https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/debug/electron-browser/callStackView.ts#L82

Including @testforstephen to see if this can be fixed on the Java extension side. Even though vscode UI should protect against such behavior from an extension.

rsalvador · 2018-02-23T19:43:45Z

@isidorn thank for looking into this!
vscode still blocks when changing 50ms to 200ms in callStackView.ts
the reason is because the perf problem occurs even before callStackView.ts is invoked, the code hogging the cpu seem to be the calculations between the calls to fetchThreads() in debugService.ts and the actual request of the refresh in callStackView.ts

msftclas · 2018-02-23T20:11:31Z

All CLA requirements met.

isidorn · 2018-02-26T10:10:40Z

@rsalvador thanks for more details. I will look into this in greater details next week. Until then we can also get @testforstephen feedback

testforstephen · 2018-02-27T08:25:46Z

Looks good. The fix could help to reduce the threadsRequest frequency to JVM. Especially during remote debugging, the network perf has a big impact on the response speed of each vscode debug request, reducing the threadsRequest frequency will improve the perf a lot. In a classic modern Java micro service, such as SpringBoot, they have more than 20 threads in running after started, that means there are 20 threadsRequests sent to the debugger during the debug session initialization. This give bad performance for remote debugging.

isidorn · 2018-03-06T17:19:06Z

src/vs/workbench/parts/debug/electron-browser/debugService.ts

@@ -113,6 +115,7 @@ export class DebugService implements debug.IDebugService {
 		this._onDidCustomEvent = new Emitter<debug.DebugEvent>();
 		this.sessionStates = new Map<string, debug.State>();
 		this.allProcesses = new Map<string, debug.IProcess>();
+		this.displayThreadsScheduler = new Map<string, RunOnceScheduler>();


I would prefer if this was called fetchThreadsSchedulers

isidorn · 2018-03-06T17:19:19Z

src/vs/workbench/parts/debug/electron-browser/debugService.ts

@@ -300,7 +303,7 @@ export class DebugService implements debug.IDebugService {

 		this.toDisposeOnSessionEnd.get(session.getId()).push(session.onDidThread(event => {
 			if (event.body.reason === 'started') {
-				this.fetchThreads(session).done(undefined, errors.onUnexpectedError);
+				this.debouncedDisplayThreads(session);


I would prefer if this method was inlined

Also please add a one line comment about why we are debouncing

isidorn · 2018-03-06T17:19:47Z

src/vs/workbench/parts/debug/electron-browser/debugService.ts

+			scheduler = new RunOnceScheduler(() => {
+				this.fetchThreads(session).done(undefined, errors.onUnexpectedError);
+			}, 100);
+			this.displayThreadsScheduler.set(session.getId(), scheduler);


scheduler should be added to toDisposeOnSessionEnd so it gets properly disposed

isidorn · 2018-03-06T17:20:47Z

@rsalvador I have commented in the code - mostly polish.
Also you would need to resolve conflicts in order for this to be merged.
Thanks

rsalvador · 2018-03-06T17:58:47Z

@isidorn performed all changes and rebased/resolved

isidorn · 2018-03-07T10:24:28Z

@rsalvador thanks a lot. Looks good to me, let's merge it in.

rsalvador force-pushed the perf-threads branch from 9add8be to b27256c Compare February 23, 2018 05:31

rsalvador changed the title ~~Fixes #44248~~ Improve Call Stack performance with many threads, fixes #44248 Feb 23, 2018

rsalvador force-pushed the perf-threads branch from b27256c to 96f67c7 Compare February 23, 2018 05:32

rsalvador mentioned this pull request Feb 23, 2018

VS Code freezes debugging program with many threads #44248

Closed

RMacfarlane requested a review from weinand February 23, 2018 17:26

RMacfarlane assigned weinand Feb 23, 2018

weinand assigned isidorn and unassigned weinand Feb 23, 2018

weinand requested review from isidorn and removed request for weinand February 23, 2018 17:39

isidorn added this to the March 2018 milestone Feb 23, 2018

isidorn reviewed Feb 23, 2018

View reviewed changes

isidorn requested changes Mar 6, 2018

View reviewed changes

Improve Call Stack performance with many threads, fixes microsoft#44248

d527e92

rsalvador force-pushed the perf-threads branch from fa8fb8c to d527e92 Compare March 6, 2018 17:57

isidorn merged commit 195747e into microsoft:master Mar 7, 2018

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Call Stack performance with many threads, fixes #44248 #44249

Improve Call Stack performance with many threads, fixes #44248 #44249

rsalvador commented Feb 23, 2018

weinand commented Feb 23, 2018

isidorn commented Feb 23, 2018

isidorn Feb 23, 2018

isidorn Feb 23, 2018

isidorn Feb 23, 2018

isidorn Feb 23, 2018

isidorn commented Feb 23, 2018

rsalvador commented Feb 23, 2018

msftclas commented Feb 23, 2018 •

edited

Loading

isidorn commented Feb 26, 2018

testforstephen commented Feb 27, 2018

isidorn Mar 6, 2018

isidorn Mar 6, 2018

isidorn Mar 6, 2018 •

edited

Loading

isidorn Mar 6, 2018

isidorn commented Mar 6, 2018 •

edited

Loading

rsalvador commented Mar 6, 2018

isidorn commented Mar 7, 2018

Improve Call Stack performance with many threads, fixes #44248 #44249

Improve Call Stack performance with many threads, fixes #44248 #44249

Conversation

rsalvador commented Feb 23, 2018

weinand commented Feb 23, 2018

isidorn commented Feb 23, 2018

isidorn Feb 23, 2018

Choose a reason for hiding this comment

isidorn Feb 23, 2018

Choose a reason for hiding this comment

isidorn Feb 23, 2018

Choose a reason for hiding this comment

isidorn Feb 23, 2018

Choose a reason for hiding this comment

isidorn commented Feb 23, 2018

rsalvador commented Feb 23, 2018

msftclas commented Feb 23, 2018 • edited Loading

isidorn commented Feb 26, 2018

testforstephen commented Feb 27, 2018

isidorn Mar 6, 2018

Choose a reason for hiding this comment

isidorn Mar 6, 2018

Choose a reason for hiding this comment

isidorn Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

isidorn Mar 6, 2018

Choose a reason for hiding this comment

isidorn commented Mar 6, 2018 • edited Loading

rsalvador commented Mar 6, 2018

isidorn commented Mar 7, 2018

msftclas commented Feb 23, 2018 •

edited

Loading

isidorn Mar 6, 2018 •

edited

Loading

isidorn commented Mar 6, 2018 •

edited

Loading