microsoft / microsoft/vscode

Menus: MenuInfo runs the full collect+sort twice on every construction

Open
#336,495 0 comments 0 reactions 1 assignee Claimed by @sbatten View on GitHub
Dominant language
TypeScript
Stars
193k
Forks
42.4k
PR merge metrics
PR metrics pending

Description

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

- VS Code Version: 1.139.0-insider (4dbe1643e6189ba7b1bbe542cc0e56a94d9ff132)
- OS Version: macOS 14.2 (23C64)

Steps to Reproduce:

1. Open anything that resolves a menu through `IMenuService` — a tree view row's inline actions, or any context menu.
2. Profile the renderer. Both `MenuInfoSnapshot`'s constructor and `MenuInfo`'s constructor appear, each performing a full `MenuRegistry.getMenuItems` copy + sort + context-key collection for the same `MenuId`.

### The problem

`MenuInfoSnapshot`'s constructor calls `this.refresh()` (`src/vs/platform/actions/common/menuService.ts:175`). `MenuInfo` extends it and calls `this.refresh()` again immediately after `super()` (`menuService.ts:266`):

```ts
class MenuInfoSnapshot {
constructor(_id, _collectContextKeysForSubmenus) { this.refresh(); }
}

class MenuInfo extends MenuInfoSnapshot {
constructor(...) { super(_id, _collectContextKeysForSubmenus); this.refresh(); }
}
```

Because `_sort` is virtually dispatched, the base constructor's `refresh()` resolves `this._sort` to `MenuInfo.prototype._sort` (`menuService.ts:314`) rather than the no-op base implementation at `menuService.ts:225` — so **both passes perform the full sort**, not just the cheaper collection walk. The first pass's `_menuGroups` is then discarded by the second.

This looks like a regression from 603b0ee03b8 (#219964, "Don't listen on menu changed, method 2"), which extracted `MenuInfoSnapshot`, moved `refresh()` into the base constructor, and left the derived call in place.

### Impact

In a CPU profile of a tree-view interaction on a workspace with many `view/item/context` contributions, `MenuInfoSnapshot`'s constructor accounts for **7,381ms (35.3%)** of a single 20,904ms renderer task — all of it thrown away. `MenuInfo`'s constructor totals 14,268ms (68.3%).

Full profile and surrounding context in #336496.

### Suggested fix

Remove the redundant `this.refresh()` at `menuService.ts:266`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.