Skip to content

Commit

Permalink
Fixes view node memory leaks
Browse files Browse the repository at this point in the history 
 - Adds weakEvent function to subscribe to events that will be disposed on GC
 - Reworks children handling and dispose across many view nodes
  • Loading branch information
@eamodio
eamodio committed Oct 26, 2023
1 parent f88fcf8 commit 7cc24e4
Show file tree
Hide file tree
Showing 25 changed files with 486 additions and 364 deletions.
24 changes: 24 additions & 0 deletions src/system/event.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,27 @@ export function promisifyDeferred<T, U>(
cancel: () => cancel?.(),
};
}

export function weakEvent<T, U extends object>(
event: Event<T>,
listener: (e: T) => any,
thisArg: U,
disposables?: Disposable[],
): Disposable {
const ref = new WeakRef<U>(thisArg);

const disposable = event(
(e: T) => {
const obj = ref.deref();
if (obj != null) {
listener.call(obj, e);
} else {
disposable.dispose();
}
},
null,
disposables,
);

return disposable;
}
23 changes: 6 additions & 17 deletions src/views/nodes/autolinkedItemsNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,17 @@ import { GitUri } from '../../git/gitUri';
import type { GitLog } from '../../git/models/log';
import { PullRequest } from '../../git/models/pullRequest';
import { pauseOnCancelOrTimeoutMapTuple } from '../../system/cancellation';
import { gate } from '../../system/decorators/gate';
import { debug } from '../../system/decorators/log';
import { getSettledValue } from '../../system/promise';
import type { ViewsWithCommits } from '../viewBase';
import { AutolinkedItemNode } from './autolinkedItemNode';
import { LoadMoreNode, MessageNode } from './common';
import { PullRequestNode } from './pullRequestNode';
import { ContextValues, getViewNodeId, ViewNode } from './viewNode';
import type { ViewNode } from './viewNode';
import { CacheableChildrenViewNode, ContextValues, getViewNodeId } from './viewNode';

let instanceId = 0;

export class AutolinkedItemsNode extends ViewNode<'autolinks', ViewsWithCommits> {
export class AutolinkedItemsNode extends CacheableChildrenViewNode<'autolinks', ViewsWithCommits> {
private _instanceId: number;

constructor(
Expand All @@ -35,10 +34,8 @@ export class AutolinkedItemsNode extends ViewNode<'autolinks', ViewsWithCommits>
return this._uniqueId;
}

private _children: ViewNode[] | undefined;

async getChildren(): Promise<ViewNode[]> {
if (this._children == null) {
if (this.children == null) {
const commits = [...this.log.commits.values()];

let children: ViewNode[] | undefined;
Expand Down Expand Up @@ -92,9 +89,9 @@ export class AutolinkedItemsNode extends ViewNode<'autolinks', ViewsWithCommits>
);
}

this._children = children;
this.children = children;
}
return this._children;
return this.children;
}

getTreeItem(): TreeItem {
Expand All @@ -107,12 +104,4 @@ export class AutolinkedItemsNode extends ViewNode<'autolinks', ViewsWithCommits>

return item;
}

@gate()
@debug()
override refresh(reset: boolean = false) {
if (!reset) return;

this._children = undefined;
}
}
36 changes: 26 additions & 10 deletions src/views/nodes/branchNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { Deferred } from '../../system/promise';
import { defer, getSettledValue } from '../../system/promise';
import { pad } from '../../system/string';
import type { ViewsWithBranches } from '../viewBase';
import { disposeChildren } from '../viewBase';
import { BranchTrackingStatusNode } from './branchTrackingStatusNode';
import { CommitNode } from './commitNode';
import { LoadMoreNode, MessageNode } from './common';
Expand Down Expand Up @@ -94,6 +95,12 @@ export class BranchNode
};
}

@debug()
override dispose() {
super.dispose();
this.children = undefined;
}

override get id(): string {
return this._uniqueId;
}
Expand Down Expand Up @@ -135,9 +142,18 @@ export class BranchNode
}

private _children: ViewNode[] | undefined;
protected get children(): ViewNode[] | undefined {
return this._children;
}
protected set children(value: ViewNode[] | undefined) {
if (this._children === value) return;

disposeChildren(this._children, value);
this._children = value;
}

async getChildren(): Promise<ViewNode[]> {
if (this._children == null) {
if (this.children == null) {
const branch = this.branch;

let onCompleted: Deferred<void> | undefined;
Expand Down Expand Up @@ -171,9 +187,9 @@ export class BranchNode
clearTimeout(timeout);

// If we found a pull request, insert it into the children cache (if loaded) and refresh the node
if (pr != null && this._children != null) {
this._children.splice(
this._children[0] instanceof CompareBranchNode ? 1 : 0,
if (pr != null && this.children != null) {
this.children.splice(
this.children[0] instanceof CompareBranchNode ? 1 : 0,
0,
new PullRequestNode(this.view, this, pr, branch),
);
Expand Down Expand Up @@ -330,11 +346,11 @@ export class BranchNode
);
}

this._children = children;
this.children = children;
setTimeout(() => onCompleted?.fulfill(), 1);
}

return this._children;
return this.children;
}

async getTreeItem(): Promise<TreeItem> {
Expand Down Expand Up @@ -510,10 +526,10 @@ export class BranchNode
void this.view.refresh(true);
}

@gate()
@debug()
override refresh(reset?: boolean) {
this._children = undefined;
void super.refresh?.(reset);

this.children = undefined;
if (reset) {
this._log = undefined;
this.deleteState();
Expand Down Expand Up @@ -586,7 +602,7 @@ export class BranchNode
this._log = log;
this.limit = log?.count;

this._children = undefined;
this.children = undefined;
void this.triggerChange(false);
}
}
17 changes: 7 additions & 10 deletions src/views/nodes/branchesNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ import { ThemeIcon, TreeItem, TreeItemCollapsibleState } from 'vscode';
import { GitUri } from '../../git/gitUri';
import type { Repository } from '../../git/models/repository';
import { makeHierarchical } from '../../system/array';
import { gate } from '../../system/decorators/gate';
import { debug } from '../../system/decorators/log';
import type { ViewsWithBranchesNode } from '../viewBase';
import { BranchNode } from './branchNode';
import { BranchOrTagFolderNode } from './branchOrTagFolderNode';
import { MessageNode } from './common';
import { ContextValues, getViewNodeId, ViewNode } from './viewNode';
import type { ViewNode } from './viewNode';
import { CacheableChildrenViewNode, ContextValues, getViewNodeId } from './viewNode';

export class BranchesNode extends ViewNode<'branches', ViewsWithBranchesNode> {
export class BranchesNode extends CacheableChildrenViewNode<'branches', ViewsWithBranchesNode> {
constructor(
uri: GitUri,
view: ViewsWithBranchesNode,
Expand All @@ -31,10 +31,8 @@ export class BranchesNode extends ViewNode<'branches', ViewsWithBranchesNode> {
return this.repo.path;
}

private _children: ViewNode[] | undefined;

async getChildren(): Promise<ViewNode[]> {
if (this._children == null) {
if (this.children == null) {
const branches = await this.repo.getBranches({
// only show local branches
filter: b => !b.remote,
Expand Down Expand Up @@ -74,10 +72,10 @@ export class BranchesNode extends ViewNode<'branches', ViewsWithBranchesNode> {
);

const root = new BranchOrTagFolderNode(this.view, this, 'branch', hierarchy, this.repo.path, '', undefined);
this._children = root.getChildren();
this.children = root.getChildren();
}

return this._children;
return this.children;
}

async getTreeItem(): Promise<TreeItem> {
Expand All @@ -96,9 +94,8 @@ export class BranchesNode extends ViewNode<'branches', ViewsWithBranchesNode> {
return item;
}

@gate()
@debug()
override refresh() {
this._children = undefined;
super.refresh(true);
}
}
37 changes: 27 additions & 10 deletions src/views/nodes/commitNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@ import { makeHierarchical } from '../../system/array';
import { pauseOnCancelOrTimeoutMapTuplePromise } from '../../system/cancellation';
import { configuration } from '../../system/configuration';
import { getContext } from '../../system/context';
import { gate } from '../../system/decorators/gate';
import { debug } from '../../system/decorators/log';
import { joinPaths, normalizePath } from '../../system/path';
import type { Deferred } from '../../system/promise';
import { defer, getSettledValue } from '../../system/promise';
import { sortCompare } from '../../system/string';
import type { FileHistoryView } from '../fileHistoryView';
import type { ViewsWithCommits } from '../viewBase';
import { disposeChildren } from '../viewBase';
import { CommitFileNode } from './commitFileNode';
import type { FileNode } from './folderNode';
import { FolderNode } from './folderNode';
Expand Down Expand Up @@ -49,6 +50,12 @@ export class CommitNode extends ViewRefNode<'commit', ViewsWithCommits | FileHis
this._uniqueId = getViewNodeId(this.type, this.context);
}

@debug()
override dispose() {
super.dispose();
this.children = undefined;
}

override get id(): string {
return this._uniqueId;
}
Expand All @@ -65,13 +72,22 @@ export class CommitNode extends ViewRefNode<'commit', ViewsWithCommits | FileHis
return this.commit;
}

private _children: (PullRequestNode | FileNode)[] | undefined;
private _children: ViewNode[] | undefined;
protected get children(): ViewNode[] | undefined {
return this._children;
}
protected set children(value: ViewNode[] | undefined) {
if (this._children === value) return;

disposeChildren(this._children, value);
this._children = value;
}

async getChildren(): Promise<ViewNode[]> {
if (this._children == null) {
if (this.children == null) {
const commit = this.commit;

let children: (PullRequestNode | FileNode)[] = [];
let children: ViewNode[] = [];
let onCompleted: Deferred<void> | undefined;
let pullRequest;

Expand Down Expand Up @@ -101,8 +117,8 @@ export class CommitNode extends ViewRefNode<'commit', ViewsWithCommits | FileHis
clearTimeout(timeout);

// If we found a pull request, insert it into the children cache (if loaded) and refresh the node
if (pr != null && this._children != null) {
this._children.unshift(new PullRequestNode(this.view, this, pr, commit));
if (pr != null && this.children != null) {
this.children.unshift(new PullRequestNode(this.view, this, pr, commit));
}

// Refresh this node to add the pull request node or remove the spinner
Expand Down Expand Up @@ -136,11 +152,11 @@ export class CommitNode extends ViewRefNode<'commit', ViewsWithCommits | FileHis
children.unshift(new PullRequestNode(this.view, this, pullRequest, commit));
}

this._children = children;
this.children = children;
setTimeout(() => onCompleted?.fulfill(), 1);
}

return this._children;
return this.children;
}

async getTreeItem(): Promise<TreeItem> {
Expand Down Expand Up @@ -197,9 +213,10 @@ export class CommitNode extends ViewRefNode<'commit', ViewsWithCommits | FileHis
};
}

@gate()
override refresh(reset?: boolean) {
this._children = undefined;
void super.refresh?.(reset);

this.children = undefined;
if (reset) {
this.deleteState();
}
Expand Down
Loading

0 comments on commit 7cc24e4

Please sign in to comment.