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

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

Merged
merged 1 commit into from Mar 7, 2018

Conversation

Projects
None yet
5 participants
@rsalvador
Copy link
Contributor

rsalvador commented Feb 23, 2018

No description provided.

@rsalvador rsalvador force-pushed the rsalvador:perf-threads branch from 9add8be to b27256c Feb 23, 2018

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

@rsalvador rsalvador force-pushed the rsalvador:perf-threads branch from b27256c to 96f67c7 Feb 23, 2018

@RMacfarlane RMacfarlane requested a review from weinand Feb 23, 2018

@weinand weinand assigned isidorn and unassigned weinand Feb 23, 2018

@weinand weinand requested review from isidorn and removed request for weinand Feb 23, 2018

@weinand

This comment has been minimized.

Copy link
Member

weinand commented Feb 23, 2018

@isidorn change looks good and makes sense to me.

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Feb 23, 2018

@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 isidorn added this to the March 2018 milestone Feb 23, 2018

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

This comment has been minimized.

@isidorn

isidorn Feb 23, 2018

Contributor

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

@@ -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>;

This comment has been minimized.

@isidorn

isidorn Feb 23, 2018

Contributor

Use lowercase string

@@ -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>();

This comment has been minimized.

@isidorn

isidorn Feb 23, 2018

Contributor

Use lowercase string

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

This comment has been minimized.

@isidorn

isidorn Feb 23, 2018

Contributor

This will make

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Feb 23, 2018

@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

This comment has been minimized.

Copy link
Contributor

rsalvador commented Feb 23, 2018

@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

This comment has been minimized.

Copy link

msftclas commented Feb 23, 2018

CLA assistant check
All CLA requirements met.

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Feb 26, 2018

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

@testforstephen

This comment has been minimized.

Copy link

testforstephen commented Feb 27, 2018

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.

@@ -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>();

This comment has been minimized.

@isidorn

isidorn Mar 6, 2018

Contributor

I would prefer if this was called fetchThreadsSchedulers

@@ -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);

This comment has been minimized.

@isidorn

isidorn Mar 6, 2018

Contributor

I would prefer if this method was inlined

This comment has been minimized.

@isidorn

isidorn Mar 6, 2018

Contributor

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

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

This comment has been minimized.

@isidorn

isidorn Mar 6, 2018

Contributor

scheduler should be added to toDisposeOnSessionEnd so it gets properly disposed

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 6, 2018

@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 rsalvador force-pushed the rsalvador:perf-threads branch from fa8fb8c to d527e92 Mar 6, 2018

@rsalvador

This comment has been minimized.

Copy link
Contributor

rsalvador commented Mar 6, 2018

@isidorn performed all changes and rebased/resolved

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 7, 2018

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

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

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment