fix: memory leak due to design token bindings

[m-akinc]

🐛 Bug Report

Given a component whose template uses a derived token value, as instances of that component are replaced in the DOM, the old, detached instances cannot be garbage collected, i.e. are leaked.

💻 Repro or Code Sample

See this codepen. Each time you click the button, it gets replaced by a new button instance. The old buttons, which are no longer referenced from the code or DOM, are permanently leaked. See below:

🤔 Expected Behavior

Detached components should be garbage collected.

😯 Current Behavior

Memory is leaked.

💁 Possible Solution

The cause seems to be that DesignTokenNodes do not tear down their binding observers when their corresponding nodes are detached. I.e. this problem is resolved by adding the below for loop to DesignTokenNode.unbind():

    /**
     * Invoked when the DesignTokenNode.target is detached from the document
     */
    public unbind(): void {
        if (this.parent) {
            const parent = childToParent.get(this)!;
            parent.removeChild(this);
        }
        for (const token of this.bindingObservers.keys()) {
            this.tearDownBindingObserver(token);
        }
    }

🔦 Context

We have a custom table component with row components that are replaced when scrolled in and out of view. This quickly eats up large amounts of memory.

[chrisdholt]

Thanks @m-akinc - given the nature of this (memory leak), we'd welcome a PR to FE1 to help resolve this for you.

[m-akinc]

Thanks @m-akinc - given the nature of this (memory leak), we'd welcome a PR to FE1 to help resolve this for you.

Here's that PR: #7023

[rajsite]

@chrisdholt we have validated the memory leak issues are resolved with #7023 included in @microsoft/[email protected].

We did find changes to performance in browsers as a result of the changes but with some investigation we are pretty confident the changes are representative of the GC being able to do more collections enabled by the leak clean-up (observed changes in performance seem to be related to GC pauses and not changes in script runtime).

We have not (yet?) hit the stack overflow regression discussed on discord but as there are multiple changes included in the @microsoft/[email protected] release additional investigation would be needed to determine if there are other bugfixes warranted.

I think this issue can can be closed and further investigations would be separate issues.

[chrisdholt]

@chrisdholt we have validated the memory leak issues are resolved with #7023 included in @microsoft/[email protected].

We did find changes to performance in browsers as a result of the changes but with some investigation we are pretty confident the changes are representative of the GC being able to do more collections enabled by the leak clean-up (observed changes in performance seem to be related to GC pauses and not changes in script runtime).

We have not (yet?) hit the stack overflow regression discussed on discord but as there are multiple changes included in the @microsoft/[email protected] release additional investigation would be needed to determine if there are other bugfixes warranted.

I think this issue can can be closed and further investigations would be separate issues.

Thanks for following up here!