Skip to content

fix: memory leak in notebook baseCellViewModel#205499

Merged
rebornix merged 4 commits intomicrosoft:mainfrom
SimonSiefke:fix/memory-leak-notebook-cell-view-model
Mar 6, 2024
Merged

fix: memory leak in notebook baseCellViewModel#205499
rebornix merged 4 commits intomicrosoft:mainfrom
SimonSiefke:fix/memory-leak-notebook-cell-view-model

Conversation

@SimonSiefke
Copy link
Contributor

@SimonSiefke SimonSiefke commented Feb 18, 2024

Helps with #186361

Disposable output

In the scrolling up and down notebook test I noticed lots of CodeCellViewModel._register disposables:

"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",

More details

// baseCellViewModel.ts
async resolveTextModel(): Promise<model.ITextModel> {
	if (!this._textModelRef || !this.textModel) {
		this._textModelRef = await this._modelService.createModelReference(this.uri);
		if (this._isDisposed) {
			return this.textModel!;
		}

		if (!this._textModelRef) {
			throw new Error(`Cannot resolve text model for ${this.uri}`);
		}

		this._register(this.textModel!.onDidChangeContent(() => this.onDidChangeTextModelContent()));
	}

	return this.textModel!;
}

In baseCellViewModel, a text model change listener is registered when resolving a text model. The listener is only disposed when the viewModel is disposed, but not when the text model is detached. Because baseCellViewModel is reused, attaching and detaching many different text editors when scrolling up and down, more change listeners are registered over time.

It seems the listeners would need to be unregistered when detaching a text editor, similar to how textModelRef is disposed when detaching a text editor:

detachTextEditor() {
	if (this._textModelRef) {
		this._textModelRef.dispose();
		this._textModelRef = undefined;
	}
	this._textModelRefChangeDisposable?.dispose();
	this._textModelRefChangeDisposable = undefined;
}

Test script

For testing, I ran the following test script (replace VSCODE_PATH with path to local vscode):

git clone git@github.com:SimonSiefke/vscode-memory-leak-finder.git &&
cd vscode-memory-leak-finder &&
git checkout v5.42.0 &&
npm ci &&
VSCODE_PATH="/home/simon/.cache/repos/vscode/scripts/code.sh"  node packages/cli/bin/test.js --cwd packages/e2e  --check-leaks --measure-after --measure growing-disposable-stores --only notebook.code-scrolling --runs 97 &&
cat .vscode-memory-leak-finder-results/growing-disposable-stores/notebook.code-scrolling.json

Results for disposable counts

In the test output, the number of CodeCellViewModel._register disposables when scrolling up and down 97 times was reduced from 1773 to 0.

@bpasero
Copy link
Member

bpasero commented Feb 19, 2024

Fyi for this I find MutableDisposable to be a good fit because it allows you to register the overall disposable and make sure it gets disposed properly but then also allows to set the value each time it changes, disposing the previous one.

@roblourens roblourens assigned rebornix and unassigned roblourens Feb 19, 2024
@SimonSiefke
Copy link
Contributor Author

SimonSiefke commented Feb 19, 2024

Thank you! I updated the implementation to use MutableDisposable and it's indeed simpler now! :)

@rebornix
Copy link
Member

rebornix commented Feb 20, 2024

Thank you!

@vscodenpa vscodenpa added this to the February 2024 milestone Feb 20, 2024
@aiday-mar
Copy link
Contributor

aiday-mar commented Feb 23, 2024

I am moving this PR to the March 2024 milestone, feel free to change it back if you would like to make the corresponding issue a candidate.

@aiday-mar aiday-mar modified the milestones: February 2024, March 2024 Feb 23, 2024
@rebornix rebornix merged commit ea142b5 into microsoft:main Mar 6, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
@SimonSiefke SimonSiefke deleted the fix/memory-leak-notebook-cell-view-model branch January 15, 2026 16:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rebornix rebornix rebornix approved these changes

@bpasero bpasero bpasero approved these changes

Assignees

@rebornix rebornix

Labels

None yet

Projects

None yet

Milestone

March 2024

Development

Successfully merging this pull request may close these issues.

6 participants

@SimonSiefke @bpasero @rebornix @aiday-mar @roblourens @vscodenpa