debug: Fix UI freezing on "continue" with high number of threads #283635
Conversation
@microsoft-github-policy-service agree company="Meta" |
|
|
||
| if (needFreshFetchThreads) { | ||
| this.fetchThreadsScheduler.rawValue?.cancel(); | ||
| await this.fetchThreads(); |
There was a problem hiding this comment.
We don't want to await outside the statusQueue since that opens up to race conditions (e.g. DA continues threads and pauses again before this fetchThreads finishes and we end in a torn state)
I think we probably just want to debounce the _onDidChangeCallStack.fire call in DebugModel.clearThreads. You can see how we do that with the fetchThreadsScheduler already today in the debug session, we'd do the same thing there.
Btw reviewing the code I notice a bug in DebugModel.clearThreads -- we don't check the reference (thread ID) when clearing the schedulers. If a thread ID is set, we should only clear its scheduler, not all schedulers. If you're up for fixing a low hanging bug there, feel free to do so 🙂
This fixes a performance regression accidentally introduced in microsoft#265755. Before that change, if the `continued` event had `allThreadsContinued` set, then we'd call `this.model.clearThreads()` passing `undefined` as `threadId`, which would update all threads in one pass. After that change, a call to `this.model.clearThreads()` would be done for each thread. Because each call to `this.model.clearThreads()` ends up calling `this._onDidChangeCallStack.fire()`, we end up with quadratic overhead as some of the handlers traverse all threads in the call-stack box. This would lead to minute long freezes when using the Erlang debugger, to debug a system with ~20K Erlang processes. We now debounce the calls to `_onDidChangeCallStack.fire()` to avoid the issue.
When calling `clearThreads()` on the model, we'd cancel every scheduled action, for every session, instead of those relevant only for the given session and, optionally, thread.
00a2256 to
d96d639
Compare
|
@connor4312 thanks for the review! I've updated the branch with a new version that instead debounces the calls, and did a drive-by fix of the bug you spotted in |
|
|
||
| session.clearThreads(removeThreads, reference); | ||
| this._onDidChangeCallStack.fire(undefined); | ||
| this._onDidChangeCallStackFire.schedule(); |
There was a problem hiding this comment.
Small thing: we should schedule only if not already scheduled. Otherwise a DA with a ton of threads and very frequent events could result in the call stack never updating.
| private _onDidChangeCallStackFire = new RunOnceScheduler(() => { | ||
| this._onDidChangeCallStack.fire(undefined); | ||
| }, 100); |
There was a problem hiding this comment.
| private _onDidChangeCallStackFire = new RunOnceScheduler(() => { | |
| this._onDidChangeCallStack.fire(undefined); | |
| }, 100); | |
| private _onDidChangeCallStackFire = this._register(new RunOnceScheduler(() => { | |
| this._onDidChangeCallStack.fire(undefined); | |
| }, 100)); |
|
Added a fixup for both issues |
|
thanks for the contribution! |
…rosoft#283635) * debug: Fix UI freezing on "continue" with high number of threads This fixes a performance regression accidentally introduced in microsoft#265755. Before that change, if the `continued` event had `allThreadsContinued` set, then we'd call `this.model.clearThreads()` passing `undefined` as `threadId`, which would update all threads in one pass. After that change, a call to `this.model.clearThreads()` would be done for each thread. Because each call to `this.model.clearThreads()` ends up calling `this._onDidChangeCallStack.fire()`, we end up with quadratic overhead as some of the handlers traverse all threads in the call-stack box. This would lead to minute long freezes when using the Erlang debugger, to debug a system with ~20K Erlang processes. We now debounce the calls to `_onDidChangeCallStack.fire()` to avoid the issue. * debug: Avoid cancelling too many scheduled actions when clearing threads When calling `clearThreads()` on the model, we'd cancel every scheduled action, for every session, instead of those relevant only for the given session and, optionally, thread. * fixup! debug: Fix UI freezing on "continue" with high number of threads
This fixes a performance regression accidentally introduced in #265755.
Before that change, if the
continuedevent hadallThreadsContinuedset, then we'd callthis.model.clearThreads()passingundefinedasthreadId, which would update all threads in one pass. After that change, a call tothis.model.clearThreads()would be done for each thread.Because each call to
this.model.clearThreads()ends up callingthis._onDidChangeCallStack.fire(), we end up with quadratic overhead as some of the handlers traverse all threads in the call-stack box. This would lead to minute long freezes when using the Erlang debugger, to debug a system with ~20K Erlang processes.The solution is to go back to the previous implementation, where
clearThreads()is ever called with one thread-id or withundefined, but first wait onthis.fetchThreads()if needed