Menus: MenuInfo runs the full collect+sort twice on every construction
- 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
Assessment
This issue has not been assessed yet.