reduce impact of memory leaks related to editor

[SimonSiefke]

Helps with #218077

Changes

This PR changes the editor dispose function to recursively clear all child elements of the editor dom node.

DOM nodes hold strong references to parent node, sibling nodes and child nodes. When a memory leak occurs in a child component of the editor (e.g. Minimap, GlyphMarginWidgets, TextAreaHandler, ...) the editor dom (linked to the child dom) is leaked as well.

By recursively clearing all editor child elements, only the child elements will be leaked.

Reducing impact of memory leaks

While this doesn't fix any memory leak, it reduces the impact of memory leaks related to the editor. For example, when opening an editor 197 times, the number of leaked dom nodes is much lower than before:

Metric name	Before	After
detached canvas element count	~800	0
detached dom node count	14307	937
leaked object count	~16400	~2000

Making it easier to find the cause of editor memory leaks

With less leaked dom nodes, it could make it easier to find the cause of memory leaks related to the editor. For example, these are the detached dom nodes now when opening an editor 197 times:

{
  "detachedDomNodes": {
    "after": [
      {
        "className": "HTMLDivElement",
        "description": "div.lines-content.monaco-editor-background",
        "count": 399
      },
      {
        "className": "HTMLDivElement",
        "description": "div.margin",
        "count": 399
      },
     {
        "className": "HTMLSpanElement",
        "description": "span.codicon.codicon-sync.codicon-modifier-spin",
        "count": 10
      },
      {
        "className": "HTMLDivElement",
        "description": "div.orthogonal-drag-handle.end",
        "count": 8
      },
      {
        "className": "HTMLDivElement",
        "description": "div.orthogonal-drag-handle.start",
        "count": 7
      },
      // ...
    ]
  }
}

It seems there is a memory leak related to the editor background and/or editor margin component.

Performance

Clearing all dom nodes recursively has a performance overhead of ~1ms to ~1.8 ms on my machine. On the other hand, garbage collection could potentially be faster.

Test Script for measuring canvas count

git clone [email protected]:SimonSiefke/vscode-memory-leak-finder.git &&
cd vscode-memory-leak-finder &&
git checkout v5.53.0 &&
npm ci &&
VSCODE_PATH="yourPathLocalVscodeFolder/scripts/code.sh" node packages/cli/bin/test.js --cwd packages/e2e --only  editor.switch-between-tabs --check-leaks --measure-after --measure canvas-count --runs 17 &&
cat .vscode-memory-leak-finder-results/canvas-count/editor.switch-between-tabs.json 

Test Script for measuring detached dom nodes

git clone [email protected]:SimonSiefke/vscode-memory-leak-finder.git &&
cd vscode-memory-leak-finder &&
git checkout v5.53.0 &&
npm ci &&
VSCODE_PATH="yourPathLocalVscodeFolder/scripts/code.sh" node packages/cli/bin/test.js --cwd packages/e2e --only  editor.switch-between-tabs --check-leaks --measure-after --measure detached-dom-nodes --runs 17 &&
cat .vscode-memory-leak-finder-results/detached-dom-nodes/editor.switch-between-tabs.json 

Test Results

The number of canvas elements now stays pretty much constant:

[hediet]

Thanks a lot! I know there is git blame, but can you add some docs/comments to the source code that explains what problem this function solves?
Ideally, with a link to some issue/page which explains how the improvement can be verified (i.e. how the number of leaked dom elements can be figured out).
Otherwise it might not survive the next bigger refactoring.

[SimonSiefke]

Thanks for the review! I added a comment with a link to this pull request. And also updated the comment above to include the script used for measuring canvas counts and detached dom nodes.