Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Memory leak in quickpick #201320

Closed
SimonSiefke opened this issue Dec 20, 2023 · 2 comments
Closed

Memory leak in quickpick #201320

SimonSiefke opened this issue Dec 20, 2023 · 2 comments
Assignees
@TylerLeonhardt
Labels
debt Code quality issues performance quick-pick Quick-pick widget issues
Milestone
Backlog

Comments

@SimonSiefke
Copy link
Contributor

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.85.1
  • OS Version: Ubuntu 23.04

Steps to Reproduce:

  1. Open the command palette using ctrl+shift+p (or cmd+shift+p on mac)
  2. Press the escape key, closing the command palette
  3. Repeat 7 times
  4. Notice that MutableDisposable instance count has increased by 7, Action instance count has increased by 211, DisposableStore instance count has increased by 240 and Emitter instance count has increased by 302. The total disposable count has increased by 1137 from 17322 to 18459.
{
  "disposables": {
    "before": [
      {
        "name": "Emitter",
        "count": 2634,
        "location": "/src/vs/base/common/event.ts:991:13"
      },
      {
        "name": "LeakageMonitor",
        "count": 2565,
        "location": "/src/vs/base/common/event.ts:850:11"
      },
      {
        "name": "DisposableStore",
        "count": 2411,
        "location": "/src/vs/base/common/lifecycle.ts:370:1"
      },
      {
        "name": "DomListener",
        "count": 874,
        "location": "/src/vs/base/browser/dom.ts:131:13"
      },
      {
        "name": "Action",
        "count": 432,
        "location": "/src/vs/base/common/actions.ts:68:13"
      },
      {
        "name": "GlobalIdleValue",
        "count": 186,
        "location": "/src/vs/base/common/async.ts:1331:13"
      },
      {
        "name": "MutableDisposable",
        "count": 130,
        "location": "/src/vs/base/common/lifecycle.ts:511:1"
      },
      {
        "name": "DomEmitter",
        "count": 113,
        "location": "/src/vs/base/browser/event.ts:39:13"
      },
      {
        "name": "LazyTokenizationSupport",
        "count": 71,
        "location": "/src/vs/editor/common/languages.ts:1969:30"
      },
      {
        "name": "TokenizationSupportFactoryData",
        "count": 71,
        "location": "/src/vs/editor/common/tokenizationRegistry.ts:126:19"
      },
      {
        "name": "RunOnceScheduler",
        "count": 70,
        "location": "/src/vs/base/common/async.ts:916:13"
      },
      {
        "name": "DisposableMap",
        "count": 64,
        "location": "/src/vs/base/common/lifecycle.ts:718:1"
      },
      {
        "name": "Delayer",
        "count": 53,
        "location": "/src/vs/base/common/async.ts:327:20"
      },
      {
        "name": "Relay",
        "count": 52,
        "location": "/src/vs/base/common/event.ts:1589:0"
      },
      {
        "name": "CancellationTokenSource",
        "count": 43,
        "location": "/src/vs/base/common/cancellation.ts:102:13"
      },
      {
        "name": "DecorationCSSRules",
        "count": 41,
        "location": "/src/vs/editor/browser/services/abstractCodeEditorService.ts:617:13"
      },
      {
        "name": "MutableToken",
        "count": 41,
        "location": "/src/vs/base/common/cancellation.ts:60:0"
      },
      {
        "name": "ThrottledDelayer",
        "count": 38,
        "location": "/src/vs/base/common/async.ts:402:13"
      },
      {
        "name": "Throttler",
        "count": 38,
        "location": "/src/vs/base/common/async.ts:178:1"
      },
      {
        "name": "FocusTracker",
        "count": 37,
        "location": "/src/vs/base/browser/dom.ts:1240:13"
      },
      {
        "name": "TimeoutTimer",
        "count": 32,
        "location": "/src/vs/base/common/async.ts:843:13"
      },
      {
        "name": "DebounceEmitter",
        "count": 28,
        "location": "/src/vs/base/common/event.ts:1356:13"
      },
      {
        "name": "MenuImpl",
        "count": 26,
        "location": "/src/vs/platform/actions/common/menuService.ts:335:2"
      },
      {
        "name": "DragAndDropObserver",
        "count": 26,
        "location": "/src/vs/base/browser/dom.ts:2075:30"
      }
    ],
    "after": [
      {
        "name": "Emitter",
        "count": 2936,
        "location": "/src/vs/base/common/event.ts:991:13"
      },
      {
        "name": "LeakageMonitor",
        "count": 2867,
        "location": "/src/vs/base/common/event.ts:850:11"
      },
      {
        "name": "DisposableStore",
        "count": 2651,
        "location": "/src/vs/base/common/lifecycle.ts:370:1"
      },
      {
        "name": "DomListener",
        "count": 874,
        "location": "/src/vs/base/browser/dom.ts:131:13"
      },
      {
        "name": "Action",
        "count": 643,
        "location": "/src/vs/base/common/actions.ts:68:13"
      },
      {
        "name": "GlobalIdleValue",
        "count": 186,
        "location": "/src/vs/base/common/async.ts:1331:13"
      },
      {
        "name": "MutableDisposable",
        "count": 137,
        "location": "/src/vs/base/common/lifecycle.ts:511:1"
      },
      {
        "name": "DomEmitter",
        "count": 113,
        "location": "/src/vs/base/browser/event.ts:39:13"
      },
      {
        "name": "LazyTokenizationSupport",
        "count": 71,
        "location": "/src/vs/editor/common/languages.ts:1969:30"
      },
      {
        "name": "TokenizationSupportFactoryData",
        "count": 71,
        "location": "/src/vs/editor/common/tokenizationRegistry.ts:126:19"
      },
      {
        "name": "RunOnceScheduler",
        "count": 70,
        "location": "/src/vs/base/common/async.ts:916:13"
      },
      {
        "name": "CancellationTokenSource",
        "count": 64,
        "location": "/src/vs/base/common/cancellation.ts:102:13"
      },
      {
        "name": "DisposableMap",
        "count": 64,
        "location": "/src/vs/base/common/lifecycle.ts:718:1"
      },
      {
        "name": "MutableToken",
        "count": 62,
        "location": "/src/vs/base/common/cancellation.ts:60:0"
      },
      {
        "name": "Delayer",
        "count": 53,
        "location": "/src/vs/base/common/async.ts:327:20"
      },
      {
        "name": "Relay",
        "count": 52,
        "location": "/src/vs/base/common/event.ts:1589:0"
      },
      {
        "name": "DecorationCSSRules",
        "count": 41,
        "location": "/src/vs/editor/browser/services/abstractCodeEditorService.ts:617:13"
      },
      {
        "name": "ThrottledDelayer",
        "count": 38,
        "location": "/src/vs/base/common/async.ts:402:13"
      },
      {
        "name": "Throttler",
        "count": 38,
        "location": "/src/vs/base/common/async.ts:178:1"
      },
      {
        "name": "FocusTracker",
        "count": 37,
        "location": "/src/vs/base/browser/dom.ts:1240:13"
      },
      {
        "name": "TimeoutTimer",
        "count": 32,
        "location": "/src/vs/base/common/async.ts:843:13"
      },
      {
        "name": "DebounceEmitter",
        "count": 28,
        "location": "/src/vs/base/common/event.ts:1356:13"
      },
      {
        "name": "MenuImpl",
        "count": 26,
        "location": "/src/vs/platform/actions/common/menuService.ts:335:2"
      },
      {
        "name": "DragAndDropObserver",
        "count": 26,
        "location": "/src/vs/base/browser/dom.ts:2075:30"
      }
    ]
  }
}

Test script

git clone git@github.com:SimonSiefke/vscode-memory-leak-finder.git &&
cd vscode-memory-leak-finder &&
git checkout v5.34.0 &&
npm ci &&
node packages/cli/bin/test.js --cwd packages/e2e  --check-leaks --measure-after --measure disposables --runs 7  --only quick-pick.toggle &&
cat .vscode-memory-leak-finder-results/disposables/quick-pick.toggle.json

Additional Information

One class that has increased the most in the above test was Action. Looking where Actions relates to the quick input I found this code in /quickInput.ts where actions are created:

if (this.buttonsUpdated) {
			this.buttonsUpdated = false;
			this.ui.leftActionBar.clear();
			const leftButtons = this.buttons.filter(button => button === backButton);
			this.ui.leftActionBar.push(leftButtons.map((button, index) => {
				const action = new Action(`id-${index}`, '', button.iconClass || getIconClass(button.iconPath), true, async () => {
					this.onDidTriggerButtonEmitter.fire(button);
				});
				action.tooltip = button.tooltip || '';
				return action;
			}), { icon: true, label: false });
			this.ui.rightActionBar.clear();
			const rightButtons = this.buttons.filter(button => button !== backButton);
			this.ui.rightActionBar.push(rightButtons.map((button, index) => {
				const action = new Action(`id-${index}`, '', button.iconClass || getIconClass(button.iconPath), true, async () => {
					this.onDidTriggerButtonEmitter.fire(button);
				});
				action.tooltip = button.tooltip || '';
				return action;
			}), { icon: true, label: false });
		}

Perhaps it could be this code that causes the Actions to be created. It seems the actions aren't registered/disposed like other disposables.
TylerLeonhardt added a commit that referenced this issue Dec 20, 2023
@TylerLeonhardt
Don't use disposable Action and use IAction instead
7856c18 
Turns out all of these disposables were not getting cleaned up... but they really don't need to be disposables in this context.

ref #201320
@TylerLeonhardt TylerLeonhardt mentioned this issue Dec 20, 2023
Merged
TylerLeonhardt added a commit that referenced this issue Dec 20, 2023
@TylerLeonhardt
Don't use disposable Action and use IAction instead (#201337)
468f6fc 
Turns out all of these disposables were not getting cleaned up... but they really don't need to be disposables in this context.

ref #201320
@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Dec 20, 2023 •
edited
Loading

Thanks for reporting this! I think I've cleaned up the Actions... now we use the simpler non-disposable kind... I'll chip away at the rest over time.

@TylerLeonhardt TylerLeonhardt added debt Code quality issues quick-pick Quick-pick widget issues performance labels Dec 20, 2023
@TylerLeonhardt TylerLeonhardt added this to the Backlog milestone Dec 20, 2023
@SimonSiefke
Copy link
Contributor Author

SimonSiefke commented Dec 21, 2023 •
edited
Loading

Thanks a lot, @TylerLeonhardt!

I see now:

  • No more Actions are leaked (211 -> 0)
  • Disposable stores are 211 less leaked (240 -> 29)
  • Emitter are 211 less leaked (302 -> 91)
  • MutableDisposable instance count still increases (7 -> 7)
{
  "disposables": {
    "before": [
      {
        "name": "Emitter",
        "count": 2457,
        "location": "/src/vs/base/common/event.ts:991:13"
      },
      {
        "name": "LeakageMonitor",
        "count": 2389,
        "location": "/src/vs/base/common/event.ts:850:11"
      },
      {
        "name": "DisposableStore",
        "count": 2301,
        "location": "/src/vs/base/common/lifecycle.ts:370:1"
      },
      {
        "name": "DomListener",
        "count": 911,
        "location": "/src/vs/base/browser/dom.ts:131:13"
      },
      {
        "name": "Action",
        "count": 414,
        "location": "/src/vs/base/common/actions.ts:68:13"
      },
      {
        "name": "GlobalIdleValue",
        "count": 186,
        "location": "/src/vs/base/common/async.ts:1330:13"
      },
      {
        "name": "MutableDisposable",
        "count": 134,
        "location": "/src/vs/base/common/lifecycle.ts:511:1"
      },
      {
        "name": "DomEmitter",
        "count": 110,
        "location": "/src/vs/base/browser/event.ts:39:13"
      },
      {
        "name": "LazyTokenizationSupport",
        "count": 71,
        "location": "/src/vs/editor/common/languages.ts:1970:30"
      },
      {
        "name": "TokenizationSupportFactoryData",
        "count": 71,
        "location": "/src/vs/editor/common/tokenizationRegistry.ts:126:19"
      },
      {
        "name": "RunOnceScheduler",
        "count": 70,
        "location": "/src/vs/base/common/async.ts:915:13"
      },
      {
        "name": "DisposableMap",
        "count": 64,
        "location": "/src/vs/base/common/lifecycle.ts:718:1"
      },
      {
        "name": "Delayer",
        "count": 52,
        "location": "/src/vs/base/common/async.ts:327:20"
      },
      {
        "name": "Relay",
        "count": 46,
        "location": "/src/vs/base/common/event.ts:1589:0"
      },
      {
        "name": "DecorationCSSRules",
        "count": 41,
        "location": "/src/vs/editor/browser/services/abstractCodeEditorService.ts:617:13"
      },
      {
        "name": "ThrottledDelayer",
        "count": 37,
        "location": "/src/vs/base/common/async.ts:402:13"
      },
      {
        "name": "Throttler",
        "count": 37,
        "location": "/src/vs/base/common/async.ts:178:1"
      },
      {
        "name": "FocusTracker",
        "count": 37,
        "location": "/src/vs/base/browser/dom.ts:1256:13"
      },
      {
        "name": "CancellationTokenSource",
        "count": 34,
        "location": "/src/vs/base/common/cancellation.ts:102:13"
      },
      {
        "name": "MutableToken",
        "count": 32,
        "location": "/src/vs/base/common/cancellation.ts:60:0"
      },
      {
        "name": "TimeoutTimer",
        "count": 32,
        "location": "/src/vs/base/common/async.ts:842:13"
      },
      {
        "name": "DebounceEmitter",
        "count": 28,
        "location": "/src/vs/base/common/event.ts:1356:13"
      },
      {
        "name": "MenuImpl",
        "count": 26,
        "location": "/src/vs/platform/actions/common/menuService.ts:335:2"
      },
      {
        "name": "DragAndDropObserver",
        "count": 26,
        "location": "/src/vs/base/browser/dom.ts:2092:30"
      },
      {
        "name": "EventMultiplexer",
        "count": 25,
        "location": "/src/vs/base/common/event.ts:1434:1"
      },
      {
        "name": "ScrollbarVisibilityController",
        "count": 21,
        "location": "/src/vs/base/browser/ui/scrollbar/scrollbarVisibilityController.ts:22:13"
      },
      {
        "name": "ActionBar",
        "count": 21,
        "location": "/src/vs/base/browser/ui/actionbar/actionbar.ts:109:13"
      },
      {
        "name": "GlobalPointerMoveMonitor",
        "count": 21,
        "location": "/src/vs/base/browser/globalPointerMoveMonitor.ts:17:0"
      },
      {
        "name": "ActionRunner",
        "count": 20,
        "location": "/src/vs/base/common/actions.ts:168:0"
      },
      {
        "name": "ViewContainerActivityAction",
        "count": 19,
        "location": "/src/vs/workbench/browser/parts/paneCompositeBar.ts:713:2"
      },
      {
        "name": "ToggleCompositeBadgeAction",
        "count": 19,
        "location": "/src/vs/workbench/browser/parts/compositeBarActions.ts:764:10"
      }
    ],
    "after": [
      {
        "name": "Emitter",
        "count": 2548,
        "location": "/src/vs/base/common/event.ts:991:13"
      },
      {
        "name": "LeakageMonitor",
        "count": 2480,
        "location": "/src/vs/base/common/event.ts:850:11"
      },
      {
        "name": "DisposableStore",
        "count": 2330,
        "location": "/src/vs/base/common/lifecycle.ts:370:1"
      },
      {
        "name": "DomListener",
        "count": 911,
        "location": "/src/vs/base/browser/dom.ts:131:13"
      },
      {
        "name": "Action",
        "count": 414,
        "location": "/src/vs/base/common/actions.ts:68:13"
      },
      {
        "name": "GlobalIdleValue",
        "count": 186,
        "location": "/src/vs/base/common/async.ts:1330:13"
      },
      {
        "name": "MutableDisposable",
        "count": 141,
        "location": "/src/vs/base/common/lifecycle.ts:511:1"
      },
      {
        "name": "DomEmitter",
        "count": 110,
        "location": "/src/vs/base/browser/event.ts:39:13"
      },
      {
        "name": "LazyTokenizationSupport",
        "count": 71,
        "location": "/src/vs/editor/common/languages.ts:1970:30"
      },
      {
        "name": "TokenizationSupportFactoryData",
        "count": 71,
        "location": "/src/vs/editor/common/tokenizationRegistry.ts:126:19"
      },
      {
        "name": "RunOnceScheduler",
        "count": 70,
        "location": "/src/vs/base/common/async.ts:915:13"
      },
      {
        "name": "DisposableMap",
        "count": 64,
        "location": "/src/vs/base/common/lifecycle.ts:718:1"
      },
      {
        "name": "CancellationTokenSource",
        "count": 55,
        "location": "/src/vs/base/common/cancellation.ts:102:13"
      },
      {
        "name": "MutableToken",
        "count": 53,
        "location": "/src/vs/base/common/cancellation.ts:60:0"
      },
      {
        "name": "Delayer",
        "count": 52,
        "location": "/src/vs/base/common/async.ts:327:20"
      },
      {
        "name": "Relay",
        "count": 46,
        "location": "/src/vs/base/common/event.ts:1589:0"
      },
      {
        "name": "DecorationCSSRules",
        "count": 41,
        "location": "/src/vs/editor/browser/services/abstractCodeEditorService.ts:617:13"
      },
      {
        "name": "ThrottledDelayer",
        "count": 37,
        "location": "/src/vs/base/common/async.ts:402:13"
      },
      {
        "name": "Throttler",
        "count": 37,
        "location": "/src/vs/base/common/async.ts:178:1"
      },
      {
        "name": "FocusTracker",
        "count": 37,
        "location": "/src/vs/base/browser/dom.ts:1256:13"
      },
      {
        "name": "TimeoutTimer",
        "count": 32,
        "location": "/src/vs/base/common/async.ts:842:13"
      },
      {
        "name": "DebounceEmitter",
        "count": 28,
        "location": "/src/vs/base/common/event.ts:1356:13"
      },
      {
        "name": "MenuImpl",
        "count": 26,
        "location": "/src/vs/platform/actions/common/menuService.ts:335:2"
      },
      {
        "name": "DragAndDropObserver",
        "count": 26,
        "location": "/src/vs/base/browser/dom.ts:2092:30"
      },
      {
        "name": "EventMultiplexer",
        "count": 25,
        "location": "/src/vs/base/common/event.ts:1434:1"
      },
      {
        "name": "ScrollbarVisibilityController",
        "count": 21,
        "location": "/src/vs/base/browser/ui/scrollbar/scrollbarVisibilityController.ts:22:13"
      },
      {
        "name": "ActionBar",
        "count": 21,
        "location": "/src/vs/base/browser/ui/actionbar/actionbar.ts:109:13"
      },
      {
        "name": "GlobalPointerMoveMonitor",
        "count": 21,
        "location": "/src/vs/base/browser/globalPointerMoveMonitor.ts:17:0"
      },
      {
        "name": "ActionRunner",
        "count": 20,
        "location": "/src/vs/base/common/actions.ts:168:0"
      },
      {
        "name": "ViewContainerActivityAction",
        "count": 19,
        "location": "/src/vs/workbench/browser/parts/paneCompositeBar.ts:713:2"
      },
      {
        "name": "ToggleCompositeBadgeAction",
        "count": 19,
        "location": "/src/vs/workbench/browser/parts/compositeBarActions.ts:764:10"
      }
    ]
  }
}

I'll try to find more information for the MutableDisposables and DisposableStores where exactly they are created! Thanks again!

Edit: The MutableDisposables are created in pickerQuickAccess.ts:

const picksDisposable = disposables.add(new MutableDisposable());

They are kept alive by a 200ms timeout and disposed after 200ms so it's no problem:

(async () => {
	if (typeof fastAndSlowPicks.mergeDelay === 'number') {
		await timeout(fastAndSlowPicks.mergeDelay, picksToken);
		if (picksToken.isCancellationRequested) {
			return;
		}
	}

	if (!slowPicksApplied) {
		fastPicksApplied = applyPicks(fastAndSlowPicks.picks, true /* skip over empty to reduce flicker */);
	}
})(),

Just the test executing too fast / finishing before the MutableDisposables were disposed. :)

@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@TylerLeonhardt TylerLeonhardt

Labels
debt Code quality issues performance quick-pick Quick-pick widget issues
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
2 participants
@TylerLeonhardt @SimonSiefke