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.

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

fix: memory leak due to design token bindings #7022

Closed
m-akinc opened this issue Sep 11, 2024 · 4 comments · Fixed by #7023
Closed

fix: memory leak due to design token bindings #7022

m-akinc opened this issue Sep 11, 2024 · 4 comments · Fixed by #7023
Labels
status:triage New Issue - needs triage

Comments

@m-akinc
Copy link
Contributor

m-akinc commented Sep 11, 2024

🐛 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:

fast-memory-leak

🤔 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.

@m-akinc m-akinc added the status:triage New Issue - needs triage label Sep 11, 2024
@chrisdholt
Copy link
Member

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
Copy link
Contributor Author

m-akinc commented Sep 11, 2024

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

janechu pushed a commit that referenced this issue Oct 2, 2024
# Pull Request

## 📖 Description

Fixes #7022

### 🎫 Issues

#7022

## 👩‍💻 Reviewer Notes


## 📑 Test Plan

No new tests, since branch is about to be frozen anyway.

## ✅ Checklist

### General

<!--- Review the list and put an x in the boxes that apply. -->

- [x] I have included a change request file using `$ npm run change`
- [ ] I have added tests for my changes.
- [x] I have tested my changes.
- [ ] I have updated the project documentation to reflect my changes.
- [x] I have read the [CONTRIBUTING](https://github.com/microsoft/fast/blob/master/CONTRIBUTING.md) documentation and followed the [standards](https://github.com/microsoft/fast/blob/master/CODE_OF_CONDUCT.md#our-standards) for this project.

## ⏭ Next Steps

None. Does not apply to mainline.
rajsite added a commit to ni/nimble that referenced this issue Nov 5, 2024
# Pull Request

## 🤨 Rationale

Update FAST versions to include the following fixes:
- microsoft/fast#7022
- microsoft/fast#6960

And handle the following NI issues:
- Fixes #1661
- https://ni.visualstudio.com/DevCentral/_workitems/edit/2843309

## 👩‍💻 Implementation

Updates the Fast Foundation version. A test performance regression was
found in all browsers but particularly in Firefox, however [after
investigation by @m-akinc](#2456) it
was found to be an acceptable change. Fixes
#2456

## 🧪 Testing

Extensive testing done in #2456

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Mert Akinc <[email protected]>
@rajsite
Copy link

rajsite commented Nov 5, 2024

@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
Copy link
Member

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:triage New Issue - needs triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants